From 14a8d2fadb04f1a34b81b432f4e88f0d7c176856 Mon Sep 17 00:00:00 2001 From: Markus Date: Thu, 16 Jul 2026 10:34:38 +0200 Subject: [PATCH 01/29] feat(workflows): add standalone WorkflowResolver and overlay subsystem Implement PR 1 of the workflow-overlays plan: a concrete, standalone WorkflowResolver for downstream workflow extensibility without touching the Preset subsystem. - Add overlay manifest schema (Overlay, OverlayEdit, validate_overlay_yaml) - Add pure-function merge engine (find_step, apply_edit, merge_steps, validate_edits) with recursive anchor search and higher-wins semantics - Add StepListComposer and tiered layer sources (project, installed, base) - Add WorkflowResolver facade with inline HIGHER_WINS priority sorting - Add CLI verbs: workflow overlay add/set-priority/enable/disable/remove/list and workflow resolve - Wire WorkflowEngine.load_workflow through WorkflowResolver - Extend workflow add to copy optional overlays/ subdirectory from local workflow directories - Add comprehensive unit, integration, and security tests Refs: discussion #3473 (https://github.com/github/spec-kit/discussions/3473) Assisted-by: Kimi (model: opencode-go/kimi-k2.7-code, autonomous) --- docs/reference/workflows.md | 190 ++++++- src/specify_cli/workflows/_commands.py | 109 ++++ src/specify_cli/workflows/engine.py | 13 +- .../workflows/overlays/__init__.py | 80 +++ .../workflows/overlays/_commands.py | 364 +++++++++++++ .../workflows/overlays/composer.py | 105 ++++ .../workflows/overlays/layer_sources.py | 142 +++++ src/specify_cli/workflows/overlays/merge.py | 272 ++++++++++ src/specify_cli/workflows/overlays/schema.py | 174 +++++++ tests/workflows/conftest.py | 26 + tests/workflows/test_overlay_commands.py | 314 ++++++++++++ tests/workflows/test_overlay_composer.py | 106 ++++ tests/workflows/test_overlay_merge.py | 402 +++++++++++++++ tests/workflows/test_overlay_schema.py | 218 ++++++++ tests/workflows/test_overlay_security.py | 204 ++++++++ tests/workflows/test_resolver_integration.py | 485 ++++++++++++++++++ 16 files changed, 3202 insertions(+), 2 deletions(-) create mode 100644 src/specify_cli/workflows/overlays/__init__.py create mode 100644 src/specify_cli/workflows/overlays/_commands.py create mode 100644 src/specify_cli/workflows/overlays/composer.py create mode 100644 src/specify_cli/workflows/overlays/layer_sources.py create mode 100644 src/specify_cli/workflows/overlays/merge.py create mode 100644 src/specify_cli/workflows/overlays/schema.py create mode 100644 tests/workflows/conftest.py create mode 100644 tests/workflows/test_overlay_commands.py create mode 100644 tests/workflows/test_overlay_composer.py create mode 100644 tests/workflows/test_overlay_merge.py create mode 100644 tests/workflows/test_overlay_schema.py create mode 100644 tests/workflows/test_overlay_security.py create mode 100644 tests/workflows/test_resolver_integration.py diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index 16bbe0893e..37e3de7919 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -86,7 +86,195 @@ Lists workflows installed in the current project. specify workflow add ``` -Installs a workflow from the catalog, a URL (HTTPS required), or a local file path. +Installs a workflow from the catalog, a URL (HTTPS required), a local YAML file, or a local directory containing `workflow.yml`. Local workflow directories may include an optional `overlays/` subdirectory, which is installed alongside the workflow as shipped overlays. + +## Workflow Overlays + +Workflow overlays let a project extend or override an installed workflow without editing the installed `workflow.yml`. This keeps local customizations safe across `specify bundle update` or `specify workflow add` upgrades. + +When `specify workflow run ` loads a workflow, the engine composes the base workflow with all enabled overlays for that workflow id. The result is validated like any other workflow definition. + +### How Overlays Work + +An overlay is a YAML file that declares a set of edit operations against the step list of a base workflow. Overlays are applied in priority order (lowest first, highest last); at the same priority, installed overlays are applied before project overlays, so project overlays win ties. + +Overlay files live in two locations: + +| Location | Purpose | Tier | +| --- | --- | --- | +| `.specify/workflows//overlays/*.yml` | Shipped with the installed workflow | `installed-overlay` | +| `.specify/workflows/overlays//*.yml` | Project-local customizations | `project-overlay` | + +Project overlays always take precedence over installed overlays at the same priority. + +### Overlay File Format + +The recommended edit format uses the operation name as the key and the anchor step id as the value: + +```yaml +id: "my-overlay" +extends: "speckit" +priority: 10 +enabled: true +edits: + - insert_after: implement + step: + id: run-lint + type: shell + run: "ruff check src/" + + - replace: review-spec + step: + id: review-spec + type: gate + message: "Review the generated spec (overlay override)." + options: [approve, reject] + on_reject: abort +``` + +The explicit form is also supported: + +```yaml +edits: + - operation: insert_after + anchor: implement + step: + id: run-lint + type: shell + run: "ruff check src/" +``` + +#### Fields + +| Field | Required | Description | +| --- | --- | --- | +| `id` | yes | Identifier for this overlay. Used in `specify workflow overlay *` commands. Must be lowercase letters, digits, and hyphens only; no dots, underscores, path separators, or `overlays`. | +| `extends` | yes | The workflow id this overlay applies to. Uses the same safe-id format as `id`; `overlays` is reserved. | +| `priority` | yes | Integer `>= 1`. Higher priority overlays are applied later and win conflicts. | +| `enabled` | no | Boolean. Defaults to `true`. Disabled overlays are ignored. | +| `edits` | yes | Non-empty list of edit operations. | + +#### Edit Operations + +| Operation | `step` required | Effect | +| --- | --- | --- | +| `insert_after` | yes | Insert `step` immediately after the anchor step. | +| `insert_before` | yes | Insert `step` immediately before the anchor step. | +| `replace` | yes | Replace the anchor step with `step`. | +| `remove` | no | Remove the anchor step from the list. | + +The `anchor` is the `id` of a step in the base workflow. Anchors are resolved recursively inside `then`, `else`, `steps`, `cases.*`, and `default` blocks, so nested base steps can also be targeted. Fan-out templates (`step` inside a `fan-out` step) are **not** valid anchors. + +Step ids must not contain `:` — that character is reserved for engine-generated nested ids. + +### Overlay CLI Commands + +#### Add a Project Overlay + +```bash +specify workflow overlay add --priority +``` + +Validates the overlay file and copies it to `.specify/workflows/overlays//.yml`. `--priority` overrides the `priority` field in the file. + +#### List Overlays + +```bash +specify workflow overlay list +``` + +Shows enabled overlays for the workflow, ordered by resolver precedence. Disabled overlays are ignored by the resolver and are not listed. + +#### Change Priority + +```bash +specify workflow overlay set-priority +``` + +#### Enable or Disable + +```bash +specify workflow overlay disable +specify workflow overlay enable +``` + +#### Remove + +```bash +specify workflow overlay remove +``` + +Removes the project overlay file. Installed overlays shipped with a workflow are removed by `specify workflow remove `, which deletes the entire workflow directory. + +#### Inspect the Composed Workflow + +```bash +specify workflow resolve +``` + +Prints the layer stack (base + overlays) and the source attribution for each step after composition. Useful for debugging which overlay contributed or overrode a step. + +### Example: Adding Automated Linting after Implementation + +Given the built-in `speckit` workflow, create `project-overlay.yml`: + +```yaml +id: "add-lint" +extends: "speckit" +priority: 10 +edits: + - insert_after: implement + step: + id: run-lint + type: shell + run: "ruff check src/" +``` + +Install it: + +```bash +specify workflow overlay add project-overlay.yml --priority 10 +``` + +Run the workflow: + +```bash +specify workflow run speckit -i spec="Build a kanban board" +``` + +The composed workflow will now run the full SDD cycle and execute `ruff check src/` automatically after the `implement` step. + +### Example: Replacing a Gate + +```yaml +id: "skip-plan-review" +extends: "speckit" +priority: 20 +edits: + - replace: review-plan + step: + id: review-plan + type: command + command: speckit.plan + input: + args: "{{ inputs.spec }}" +``` + +Higher priority (`20`) means this overlay is applied after the `add-lint` overlay above. It replaces the `review-plan` gate with a non-interactive command. + +### Interaction with Bundles and Updates + +`specify workflow add ` copies `workflow.yml` and an optional `overlays/` subdirectory into `.specify/workflows//`. Overlays shipped this way are discovered automatically as `installed-overlay` layers. + +When an installed workflow is refreshed or reinstalled, project overlays in `.specify/workflows/overlays//` are preserved because they live outside the installed workflow directory. + +### Limitations + +- Overlays operate on the step list only. They cannot change workflow metadata (name, description, inputs, `requires`) or expression logic. +- Fan-out templates cannot be used as anchors. +- An overlay that targets a step id that does not exist in the base workflow will raise a validation error when the workflow is resolved. +- Overlays cannot target steps added by other overlays. +- Overlays cannot add new inputs or change the input schema of the base workflow. ## Remove a Workflow diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index e1d29b47d5..384f08c5c3 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -49,6 +49,13 @@ ) workflow_step_app.add_typer(workflow_step_catalog_app, name="catalog") +workflow_overlay_app = typer.Typer( + name="overlay", + help="Manage workflow overlays", + add_completion=False, +) +workflow_app.add_typer(workflow_overlay_app, name="overlay") + def _error_console(json_output: bool): """Console for error text: stderr under ``--json`` so the JSON stdout @@ -625,6 +632,17 @@ def _validate_and_install_local(yaml_path: Path, source_label: str) -> None: dest_dir.mkdir(parents=True, exist_ok=True) import shutil shutil.copy2(yaml_path, dest_dir / "workflow.yml") + + # If the source is a directory that also contains an overlays/ subdirectory, + # copy it alongside the workflow.yml so installed overlays are preserved. + source_dir = yaml_path.parent + source_overlays = source_dir / "overlays" + if source_overlays.is_dir() and not source_overlays.is_symlink(): + dest_overlays = dest_dir / "overlays" + if dest_overlays.exists(): + shutil.rmtree(dest_overlays) + shutil.copytree(source_overlays, dest_overlays) + registry.add(definition.id, { "name": definition.name, "version": definition.version, @@ -1724,6 +1742,97 @@ def workflow_step_catalog_remove( console.print(f"[green]✓[/green] Step catalog source '{removed_name}' removed") +@workflow_overlay_app.command("add") +def workflow_overlay_add_cmd( + source: Path = typer.Argument(..., help="Path to overlay YAML file"), + priority: int | None = typer.Option( + None, "--priority", help="Overlay priority (higher wins, >= 1)" + ), +): + """Add a project-local overlay for a workflow.""" + from .overlays._commands import workflow_overlay_add + + project_root = _require_specify_project() + if workflow_overlay_add(project_root, source, priority) is None: + raise typer.Exit(1) + + +@workflow_overlay_app.command("set-priority") +def workflow_overlay_set_priority_cmd( + workflow_id: str = typer.Argument(..., help="Workflow ID the overlay extends"), + overlay_id: str = typer.Argument(..., help="Overlay ID"), + priority: int = typer.Argument(..., help="New priority (>= 1)"), +): + """Set the priority of a project-local overlay.""" + from .overlays._commands import workflow_overlay_set_priority + + project_root = _require_specify_project() + if not workflow_overlay_set_priority(project_root, workflow_id, overlay_id, priority): + raise typer.Exit(1) + + +@workflow_overlay_app.command("enable") +def workflow_overlay_enable_cmd( + workflow_id: str = typer.Argument(..., help="Workflow ID the overlay extends"), + overlay_id: str = typer.Argument(..., help="Overlay ID"), +): + """Enable a project-local overlay.""" + from .overlays._commands import workflow_overlay_enable + + project_root = _require_specify_project() + if not workflow_overlay_enable(project_root, workflow_id, overlay_id): + raise typer.Exit(1) + + +@workflow_overlay_app.command("disable") +def workflow_overlay_disable_cmd( + workflow_id: str = typer.Argument(..., help="Workflow ID the overlay extends"), + overlay_id: str = typer.Argument(..., help="Overlay ID"), +): + """Disable a project-local overlay.""" + from .overlays._commands import workflow_overlay_disable + + project_root = _require_specify_project() + if not workflow_overlay_disable(project_root, workflow_id, overlay_id): + raise typer.Exit(1) + + +@workflow_overlay_app.command("remove") +def workflow_overlay_remove_cmd( + workflow_id: str = typer.Argument(..., help="Workflow ID the overlay extends"), + overlay_id: str = typer.Argument(..., help="Overlay ID"), +): + """Remove a project-local overlay.""" + from .overlays._commands import workflow_overlay_remove + + project_root = _require_specify_project() + if not workflow_overlay_remove(project_root, workflow_id, overlay_id): + raise typer.Exit(1) + + +@workflow_overlay_app.command("list") +def workflow_overlay_list_cmd( + workflow_id: str = typer.Argument(..., help="Workflow ID"), +): + """List overlays for a workflow.""" + from .overlays._commands import workflow_overlay_list + + project_root = _require_specify_project() + workflow_overlay_list(project_root, workflow_id) + + +@workflow_app.command("resolve") +def workflow_resolve_cmd( + workflow_id: str = typer.Argument(..., help="Workflow ID to resolve"), +): + """Show layer attribution for a resolved workflow.""" + from .overlays._commands import workflow_resolve + + project_root = _require_specify_project() + if workflow_resolve(project_root, workflow_id) is None: + raise typer.Exit(1) + + def register(app: typer.Typer) -> None: """Attach the workflow command group to the root Typer app.""" app.add_typer(workflow_app, name="workflow") diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index 6025c30c5c..01ea73d2ad 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -625,13 +625,24 @@ def load_workflow(self, source: str | Path) -> WorkflowDefinition: ValueError: If the workflow YAML is invalid. """ + from .overlays import WorkflowResolver + path = Path(source).expanduser() # Try as a direct file path first if path.suffix.lower() in (".yml", ".yaml") and path.is_file(): return WorkflowDefinition.from_yaml(path) - # Try as an installed workflow ID + # Try as an installed workflow ID, resolving any overlays. + resolver = WorkflowResolver(self.project_root) + try: + return resolver.resolve(str(source)) + except FileNotFoundError: + # Fall back to the direct workflow.yml path so callers still get + # the original error when the workflow id is not installed. + pass + + # Legacy direct path check for workflows installed without registry entries. installed_path = ( self.project_root / ".specify" diff --git a/src/specify_cli/workflows/overlays/__init__.py b/src/specify_cli/workflows/overlays/__init__.py new file mode 100644 index 0000000000..67929b0a6f --- /dev/null +++ b/src/specify_cli/workflows/overlays/__init__.py @@ -0,0 +1,80 @@ +"""Workflow overlay resolver — composes installed workflows from layers.""" + +from __future__ import annotations + +from pathlib import Path + +from ..engine import WorkflowDefinition +from .composer import StepListComposer +from .layer_sources import ( + BaseWorkflowSource, + InstalledOverlaySource, + Layer, + ProjectOverlaySource, +) +from .merge import ComposedStep + + +class WorkflowResolver: + """Resolves a workflow ID to its composed ``WorkflowDefinition``. + + Collects layers from three tiers: + - project-local overlays (``.specify/workflows/overlays//*.yml``) + - installed overlays shipped with a workflow (``.specify/workflows//overlays/*.yml``) + - the base workflow itself (``.specify/workflows//workflow.yml``) + + Resolution is higher-wins: overlays with higher priority are applied + later and override earlier edits on the same anchors. + """ + + def __init__(self, project_root: Path) -> None: + self.project_root = project_root + self._sources = [ + ProjectOverlaySource(project_root), + InstalledOverlaySource(project_root), + BaseWorkflowSource(project_root), + ] + self._composer = StepListComposer() + + def collect_all_layers(self, workflow_id: str) -> list[Layer]: + """Collect all layers sorted by resolver precedence. + + Higher priority wins. Ties are broken so project overlays rank above + installed overlays (they are listed first in higher-wins order). + """ + all_layers: list[Layer] = [] + for source in self._sources: + all_layers.extend(source.collect(workflow_id)) + + return sorted( + all_layers, + key=lambda layer: ( + -layer.priority, + 0 if layer.tier == "project-overlay" else 1, + layer.source, + ), + ) + + def resolve(self, workflow_id: str) -> WorkflowDefinition: + """Resolve a workflow ID to its composed definition. + + Raises: + FileNotFoundError: if the workflow cannot be found. + ValueError: if the composed workflow is invalid. + """ + layers = self.collect_all_layers(workflow_id) + definition, _ = self._composer.compose(layers) + if definition is None: + raise FileNotFoundError(f"Workflow not found: {workflow_id}") + return definition + + def resolve_with_layers( + self, workflow_id: str + ) -> tuple[WorkflowDefinition, list[Layer], list[ComposedStep]]: + """Resolve a workflow and return its definition plus layer attribution.""" + layers = self.collect_all_layers(workflow_id) + definition, attribution = self._composer.compose(layers) + if definition is None: + raise FileNotFoundError(f"Workflow not found: {workflow_id}") + return definition, layers, attribution + diff --git a/src/specify_cli/workflows/overlays/_commands.py b/src/specify_cli/workflows/overlays/_commands.py new file mode 100644 index 0000000000..bccb7872be --- /dev/null +++ b/src/specify_cli/workflows/overlays/_commands.py @@ -0,0 +1,364 @@ +"""CLI handlers for ``specify workflow overlay *`` and ``specify workflow resolve``.""" + +from __future__ import annotations + +from pathlib import Path +from typing import Any + +import typer +import yaml + +from ..._console import console, err_console +from . import WorkflowResolver +from .schema import _RESERVED_OVERLAY_WORKFLOW_IDS, _SAFE_ID_PATTERN, validate_overlay_yaml + + +def _validate_overlay_id_or_exit(id_value: str, label: str) -> None: + """Validate a single-segment overlay/workflow id from CLI arguments.""" + if not isinstance(id_value, str) or not id_value: + err_console.print(f"[red]Error:[/red] {label} is required and must be a non-empty string.") + raise typer.Exit(1) + if not _SAFE_ID_PATTERN.match(id_value): + err_console.print( + f"[red]Error:[/red] Invalid {label} {id_value!r}: " + "only lowercase letters, digits, and hyphens are allowed." + ) + raise typer.Exit(1) + + +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 workflow_id in _RESERVED_OVERLAY_WORKFLOW_IDS: + err_console.print( + f"[red]Error:[/red] Invalid workflow ID {workflow_id!r}: " + "reserved name." + ) + raise typer.Exit(1) + + +def _overlay_root(project_root: Path) -> Path: + """Return the project-local overlay root directory.""" + return project_root / ".specify" / "workflows" / "overlays" + + +def _project_overlay_dir(project_root: Path, workflow_id: str) -> Path: + """Return the project-local overlay directory for a workflow id. + + Raises typer.Exit if the resolved path escapes the overlay root. + """ + _validate_workflow_id_or_exit(workflow_id) + root = _overlay_root(project_root) + target = root / workflow_id + return _ensure_contained_dir(target, root) + + +def _ensure_contained_dir(path: Path, root: Path) -> Path: + """Ensure *path* resolves inside *root* and is not a symlink. + + Returns *path* if safe. Raises typer.Exit on traversal or symlink. + """ + if path.is_symlink(): + err_console.print( + f"[red]Error:[/red] Refusing to use symlinked path {path}." + ) + raise typer.Exit(1) + try: + resolved = path.resolve() + root_resolved = root.resolve() + resolved.relative_to(root_resolved) + except ValueError: + err_console.print( + f"[red]Error:[/red] Path traversal detected: {path} is outside the allowed directory." + ) + raise typer.Exit(1) + return path + + +def _find_overlay_file(project_root: Path, workflow_id: str, overlay_id: str) -> Path | None: + """Locate a project-local overlay file by workflow id and overlay id.""" + _validate_workflow_id_or_exit(workflow_id) + _validate_overlay_id_or_exit(overlay_id, "overlay ID") + overlay_dir = _project_overlay_dir(project_root, workflow_id) + for suffix in (".yml", ".yaml"): + candidate = overlay_dir / f"{overlay_id}{suffix}" + candidate = _ensure_contained_path(candidate, _overlay_root(project_root)) + if candidate.is_file(): + if candidate.is_symlink(): + err_console.print( + f"[red]Error:[/red] Refusing to read symlinked overlay file {candidate}." + ) + raise typer.Exit(1) + return candidate + return None + + +def _ensure_contained_path(path: Path, root: Path) -> Path: + """Return *path* only if it resolves inside *root*; otherwise raise typer.Exit.""" + try: + resolved = path.resolve() + root_resolved = root.resolve() + resolved.relative_to(root_resolved) + except ValueError: + err_console.print( + f"[red]Error:[/red] Path traversal detected: {path} is outside the allowed directory." + ) + raise typer.Exit(1) + return path + + +def _validate_priority(priority: int) -> None: + """Validate a user-supplied priority value.""" + if isinstance(priority, bool) or not isinstance(priority, int): + err_console.print("[red]Error:[/red] Priority must be an integer.") + raise typer.Exit(1) + if priority < 1: + err_console.print("[red]Error:[/red] Priority must be >= 1.") + raise typer.Exit(1) + + +def _read_overlay(path: Path) -> tuple[dict[str, Any] | None, list[str]]: + """Read and parse an overlay YAML file, returning (data, errors).""" + try: + content = path.read_text(encoding="utf-8") + except OSError as exc: + return None, [f"Failed to read {path}: {exc}"] + try: + data = yaml.safe_load(content) + except yaml.YAMLError as exc: + return None, [f"Invalid YAML in {path}: {exc}"] + if not isinstance(data, dict): + return None, [f"Overlay {path} must be a YAML mapping."] + return data, [] + + +def workflow_overlay_add( + project_root: Path, + source: Path, + priority: int | None = None, +) -> Path | None: + """Add a project-local overlay from a YAML file. + + Returns the path of the installed overlay file, or None on failure. + """ + data, errors = _read_overlay(source) + if data is None: + for err in errors: + err_console.print(f"[red]Error:[/red] {err}") + return None + + overlay, validation_errors = validate_overlay_yaml(data) + if overlay is None: + err_console.print("[red]Error:[/red] Overlay validation failed:") + for err in validation_errors: + err_console.print(f" \u2022 {err}") + return None + + if priority is not None: + _validate_priority(priority) + data["priority"] = priority + # Re-validate after mutation. + overlay, validation_errors = validate_overlay_yaml(data) + if overlay is None: + err_console.print("[red]Error:[/red] Overlay validation failed:") + for err in validation_errors: + err_console.print(f" \u2022 {err}") + return None + + target_dir = _project_overlay_dir(project_root, overlay.extends) + target_dir.mkdir(parents=True, exist_ok=True) + target_path = _ensure_contained_path( + target_dir / f"{overlay.id}.yml", _overlay_root(project_root) + ) + + try: + target_path.write_text(yaml.safe_dump(data, sort_keys=False), encoding="utf-8") + except OSError as exc: + err_console.print(f"[red]Error:[/red] Failed to write overlay: {exc}") + return None + + console.print( + f"[green]\u2713[/green] Overlay '{overlay.id}' added for workflow '{overlay.extends}'" + ) + return target_path + + +def _update_overlay_field( + project_root: Path, + workflow_id: str, + overlay_id: str, + field: str, + value: Any, +) -> bool: + """Update a single field in a project-local overlay file.""" + path = _find_overlay_file(project_root, workflow_id, overlay_id) + if path is None: + err_console.print( + f"[red]Error:[/red] Overlay '{overlay_id}' not found for workflow '{workflow_id}'" + ) + return False + + data, errors = _read_overlay(path) + if data is None: + for err in errors: + err_console.print(f"[red]Error:[/red] {err}") + return False + + data[field] = value + overlay, validation_errors = validate_overlay_yaml(data) + if overlay is None: + err_console.print("[red]Error:[/red] Overlay validation failed:") + for err in validation_errors: + err_console.print(f" \u2022 {err}") + return False + + try: + path.write_text(yaml.safe_dump(data, sort_keys=False), encoding="utf-8") + except OSError as exc: + err_console.print(f"[red]Error:[/red] Failed to write overlay: {exc}") + return False + + return True + + +def workflow_overlay_set_priority( + project_root: Path, + workflow_id: str, + overlay_id: str, + priority: int, +) -> bool: + """Set the priority of a project-local overlay.""" + _validate_priority(priority) + if _update_overlay_field(project_root, workflow_id, overlay_id, "priority", priority): + console.print( + f"[green]\u2713[/green] Priority of overlay '{overlay_id}' set to {priority}" + ) + return True + return False + + +def workflow_overlay_enable( + project_root: Path, + workflow_id: str, + overlay_id: str, +) -> bool: + """Enable a project-local overlay.""" + if _update_overlay_field(project_root, workflow_id, overlay_id, "enabled", True): + console.print(f"[green]\u2713[/green] Overlay '{overlay_id}' enabled") + return True + return False + + +def workflow_overlay_disable( + project_root: Path, + workflow_id: str, + overlay_id: str, +) -> bool: + """Disable a project-local overlay.""" + if _update_overlay_field(project_root, workflow_id, overlay_id, "enabled", False): + console.print(f"[green]\u2713[/green] Overlay '{overlay_id}' disabled") + return True + return False + + +def workflow_overlay_remove( + project_root: Path, + workflow_id: str, + overlay_id: str, +) -> bool: + """Remove a project-local overlay file.""" + path = _find_overlay_file(project_root, workflow_id, overlay_id) + if path is None: + err_console.print( + f"[red]Error:[/red] Overlay '{overlay_id}' not found for workflow '{workflow_id}'" + ) + return False + + try: + path.unlink() + except OSError as exc: + err_console.print(f"[red]Error:[/red] Failed to remove overlay: {exc}") + return False + + console.print(f"[green]\u2713[/green] Overlay '{overlay_id}' removed") + return True + + +def workflow_overlay_list(project_root: Path, workflow_id: str) -> list[dict[str, Any]]: + """List all overlays for a workflow and print a summary table. + + Returns the raw list data for machine-readable callers. + """ + _validate_workflow_id_or_exit(workflow_id) + resolver = WorkflowResolver(project_root) + layers = resolver.collect_all_layers(workflow_id) + overlays = [layer for layer in layers if layer.tier != "base"] + + if not overlays: + console.print(f"[yellow]No overlays found for workflow '{workflow_id}'.[/yellow]") + return [] + + console.print(f"Overlays for workflow '{workflow_id}':") + rows: list[dict[str, Any]] = [] + for layer in overlays: + overlay = layer.content + rows.append({ + "id": overlay.id, + "source": layer.source, + "tier": layer.tier, + "priority": overlay.priority, + "enabled": overlay.enabled, + "path": str(layer.path) if layer.path else None, + }) + enabled_marker = "enabled" if overlay.enabled else "disabled" + console.print( + f" \u2022 {overlay.id} (priority={overlay.priority}, " + f"source={layer.source}, {enabled_marker})" + ) + return rows + + +def workflow_resolve(project_root: Path, workflow_id: str) -> dict[str, Any] | None: + """Print layer attribution for a resolved workflow. + + Returns a serializable attribution payload. + """ + _validate_workflow_id_or_exit(workflow_id) + resolver = WorkflowResolver(project_root) + try: + definition, layers, attribution = resolver.resolve_with_layers(workflow_id) + except FileNotFoundError: + err_console.print( + f"[red]Error:[/red] Workflow '{workflow_id}' not found" + ) + return None + except ValueError as exc: + err_console.print(f"[red]Error:[/red] {exc}") + return None + + console.print(f"Resolved workflow '{workflow_id}':") + console.print("Layers (highest precedence first):") + for layer in layers: + console.print( + f" \u2022 [{layer.tier}] {layer.source} (priority={layer.priority})" + ) + + console.print("Step attribution:") + for composed in attribution: + console.print(f" \u2022 {composed.step_id}: {composed.source}") + + return { + "workflow_id": workflow_id, + "layers": [ + { + "source": layer.source, + "tier": layer.tier, + "priority": layer.priority, + } + for layer in layers + ], + "attribution": [ + {"step_id": composed.step_id, "source": composed.source} + for composed in attribution + ], + } diff --git a/src/specify_cli/workflows/overlays/composer.py b/src/specify_cli/workflows/overlays/composer.py new file mode 100644 index 0000000000..8ce3d16440 --- /dev/null +++ b/src/specify_cli/workflows/overlays/composer.py @@ -0,0 +1,105 @@ +"""Workflow overlay composer — builds a WorkflowDefinition from layers.""" + +from __future__ import annotations + +from typing import Any + +from ..engine import WorkflowDefinition, validate_workflow +from .layer_sources import Layer +from .merge import OverlayLayer, merge_steps, validate_edits + + +class StepListComposer: + """Compose a workflow from a base layer and overlay layers. + + - The base layer (tier="base") provides the full step list. + - Overlay layers provide edit operations. + - Overlays are applied in merge order: lowest priority first, highest last; + ties are resolved so installed overlays are applied before project + overlays, letting project overlays win. + - ``validate_workflow`` runs on the composed result. + """ + + def compose( + self, layers: list[Layer] + ) -> tuple[WorkflowDefinition | None, list]: + """Compose a ``WorkflowDefinition`` from the given layers. + + Returns ``(None, [])`` when no base layer is present. + """ + base_layer: Layer | None = None + overlay_layers: list[Layer] = [] + for layer in layers: + if layer.tier == "base": + base_layer = layer + else: + overlay_layers.append(layer) + + if base_layer is None or base_layer.path is None: + return None, [] + + # Read the base workflow definition from disk. + base_definition = WorkflowDefinition.from_yaml(base_layer.path) + base_steps = base_definition.data.get("steps", []) + if not isinstance(base_steps, list): + base_steps = [] + + # Sort overlays into merge order. + merge_order = sorted( + overlay_layers, + key=lambda layer: ( + layer.priority, + 0 if layer.tier == "installed-overlay" else 1, + layer.source, + ), + ) + + # Validate edits against base anchors before mutation. + base_step_ids = self._collect_base_step_ids(base_steps) + for layer in merge_order: + edit_errors = validate_edits(layer.content.edits, base_step_ids) + if edit_errors: + raise ValueError( + f"Overlay '{layer.content.id}' has invalid edits:\n - " + + "\n - ".join(edit_errors) + ) + + composed_steps, attribution = merge_steps( + base_steps, + [OverlayLayer(layer.content, layer.source) for layer in merge_order], + ) + + # Build composed data while preserving all non-step fields from base. + composed_data: dict[str, Any] = dict(base_definition.data) + composed_data["steps"] = composed_steps + + composed_definition = WorkflowDefinition(composed_data, source_path=base_layer.path) + validation_errors = validate_workflow(composed_definition) + if validation_errors: + # Surface validation errors as a single exception with details. + raise ValueError( + "Composed workflow is invalid:\n - " + + "\n - ".join(validation_errors) + ) + + return composed_definition, attribution + + def _collect_base_step_ids(self, steps: list[dict[str, Any]]) -> set[str]: + """Collect all base step IDs reachable in the step tree.""" + ids: set[str] = set() + for step in steps: + if not isinstance(step, dict): + continue + step_id = step.get("id") + if isinstance(step_id, str): + ids.add(step_id) + for key in ("then", "else", "steps", "default"): + nested = step.get(key) + if isinstance(nested, list): + ids.update(self._collect_base_step_ids(nested)) + cases = step.get("cases") + if isinstance(cases, dict): + for case_steps in cases.values(): + if isinstance(case_steps, list): + ids.update(self._collect_base_step_ids(case_steps)) + return ids diff --git a/src/specify_cli/workflows/overlays/layer_sources.py b/src/specify_cli/workflows/overlays/layer_sources.py new file mode 100644 index 0000000000..8f4082c612 --- /dev/null +++ b/src/specify_cli/workflows/overlays/layer_sources.py @@ -0,0 +1,142 @@ +"""Workflow overlay layer sources.""" + +from __future__ import annotations + +from dataclasses import dataclass +from pathlib import Path + +import yaml + +from .schema import Overlay, validate_overlay_yaml + + +@dataclass +class Layer: + """A single layer in the workflow overlay stack.""" + + content: Overlay + source: str + tier: str + priority: int + path: Path | None = None + + +class OverlayLoadError(ValueError): + """Raised when an overlay file cannot be loaded or validated.""" + + def __init__(self, path: Path, errors: list[str]) -> None: + self.path = path + self.errors = errors + super().__init__(f"Invalid overlay {path}:\n - " + "\n - ".join(errors)) + + +class ProjectOverlaySource: + """Project-local overlays: ``.specify/workflows/overlays//*.yml``.""" + + tier = "project-overlay" + + def __init__(self, project_root: Path) -> None: + self.project_root = project_root + self.overlays_dir = project_root / ".specify" / "workflows" / "overlays" + + def collect(self, workflow_id: str) -> list[Layer]: + """Collect all project-local overlays for the given workflow id.""" + workflow_overlay_dir = self.overlays_dir / workflow_id + if not workflow_overlay_dir.is_dir(): + return [] + layers: list[Layer] = [] + for path in sorted(workflow_overlay_dir.iterdir()): + if not path.is_file() or path.suffix not in (".yml", ".yaml"): + continue + if path.is_symlink(): + raise OverlayLoadError(path, ["Symlinked overlay files are not allowed"]) + data = yaml.safe_load(path.read_text(encoding="utf-8")) or {} + overlay, errors = validate_overlay_yaml(data) + if overlay is None or errors: + raise OverlayLoadError(path, errors) + if not overlay.enabled: + continue + if overlay.extends != workflow_id: + continue + layers.append( + Layer( + content=overlay, + source=f"project:{overlay.id}", + tier=self.tier, + priority=overlay.priority, + path=path, + ) + ) + return layers + + +class InstalledOverlaySource: + """Installed overlays: ``.specify/workflows//overlays/*.yml``.""" + + tier = "installed-overlay" + + def __init__(self, project_root: Path) -> None: + self.project_root = project_root + self.workflows_dir = project_root / ".specify" / "workflows" + + def collect(self, workflow_id: str) -> list[Layer]: + """Collect all installed overlays shipped with the given workflow.""" + installed_overlay_dir = self.workflows_dir / workflow_id / "overlays" + if not installed_overlay_dir.is_dir(): + return [] + layers: list[Layer] = [] + for path in sorted(installed_overlay_dir.iterdir()): + if not path.is_file() or path.suffix not in (".yml", ".yaml"): + continue + if path.is_symlink(): + raise OverlayLoadError(path, ["Symlinked overlay files are not allowed"]) + data = yaml.safe_load(path.read_text(encoding="utf-8")) or {} + overlay, errors = validate_overlay_yaml(data) + if overlay is None or errors: + raise OverlayLoadError(path, errors) + if not overlay.enabled: + continue + if overlay.extends != workflow_id: + continue + layers.append( + Layer( + content=overlay, + source=f"installed:{overlay.id}", + tier=self.tier, + priority=overlay.priority, + path=path, + ) + ) + return layers + + +class BaseWorkflowSource: + """Base workflow layer: ``.specify/workflows//workflow.yml``.""" + + tier = "base" + + def __init__(self, project_root: Path) -> None: + self.project_root = project_root + self.workflows_dir = project_root / ".specify" / "workflows" + + def collect(self, workflow_id: str) -> list[Layer]: + """Return the base workflow as a single layer if it exists.""" + path = self.workflows_dir / workflow_id / "workflow.yml" + if not path.is_file(): + return [] + # The base layer is represented by an Overlay with empty edits. + overlay = Overlay( + id=workflow_id, + extends=workflow_id, + priority=0, + edits=[], + ) + return [ + Layer( + content=overlay, + source="base", + tier=self.tier, + priority=0, + path=path, + ) + ] diff --git a/src/specify_cli/workflows/overlays/merge.py b/src/specify_cli/workflows/overlays/merge.py new file mode 100644 index 0000000000..73a9c9f3e6 --- /dev/null +++ b/src/specify_cli/workflows/overlays/merge.py @@ -0,0 +1,272 @@ +"""Pure-function merge engine for workflow step lists.""" + +from __future__ import annotations + +import copy +from dataclasses import dataclass +from typing import Any + +from .schema import VALID_OPERATIONS, Overlay, OverlayEdit + + +@dataclass(frozen=True) +class ComposedStep: + """Attribution tracking for a single composed step.""" + + step_id: str + source: str + + +@dataclass(frozen=True) +class OverlayLayer: + """An overlay together with its layer source for attribution.""" + + overlay: Overlay + source: str + + +# Nested step keys that may contain a list of steps. +_NESTED_LIST_KEYS = ("then", "else", "steps", "default") + + +def find_step( + steps: list[dict[str, Any]], step_id: str +) -> tuple[list[dict[str, Any]], int] | None: + """Recursively locate a step by ID and return its (parent_list, index). + + Searches flat lists and nested lists inside ``then``, ``else``, ``steps``, + ``default``, and ``cases.*``. Does *not* descend into ``fan-out`` template + steps because those are runtime-multiplied stamps, not uniquely-addressable + nodes. + """ + for i, step in enumerate(steps): + if not isinstance(step, dict): + continue + if step.get("id") == step_id: + return (steps, i) + for key in _NESTED_LIST_KEYS: + nested = step.get(key) + if isinstance(nested, list): + result = find_step(nested, step_id) + if result is not None: + return result + cases = step.get("cases") + if isinstance(cases, dict): + for case_steps in cases.values(): + if isinstance(case_steps, list): + result = find_step(case_steps, step_id) + if result is not None: + return result + return None + + +def _all_base_step_ids(steps: list[dict[str, Any]]) -> set[str]: + """Collect all step IDs reachable in a step tree (excluding fan-out templates).""" + ids: set[str] = set() + for step in steps: + if not isinstance(step, dict): + continue + step_id = step.get("id") + if isinstance(step_id, str): + ids.add(step_id) + for key in _NESTED_LIST_KEYS: + nested = step.get(key) + if isinstance(nested, list): + ids.update(_all_base_step_ids(nested)) + cases = step.get("cases") + if isinstance(cases, dict): + for case_steps in cases.values(): + if isinstance(case_steps, list): + ids.update(_all_base_step_ids(case_steps)) + return ids + + +def _init_sources_recursively( + steps: list[dict[str, Any]], sources: dict[str, str] +) -> None: + """Initialize attribution sources for all base steps, recursively.""" + for step in steps: + if not isinstance(step, dict): + continue + step_id = step.get("id") + if isinstance(step_id, str): + sources[step_id] = "base" + for key in _NESTED_LIST_KEYS: + nested = step.get(key) + if isinstance(nested, list): + _init_sources_recursively(nested, sources) + cases = step.get("cases") + if isinstance(cases, dict): + for case_steps in cases.values(): + if isinstance(case_steps, list): + _init_sources_recursively(case_steps, sources) + + +def apply_edit( + steps: list[dict[str, Any]], + edit: OverlayEdit, + source: str, +) -> tuple[list[dict[str, Any]], ComposedStep | None, str | None]: + """Apply a single edit to a step list and return the mutated list. + + The returned list is the same list object as *steps* but deep-copied first + so callers can avoid mutating the base. Raises ``ValueError`` if the anchor + is not found. + + Returns: + ``(steps, composed_step, replaced_or_removed_id)``. *composed_step* is + ``None`` for ``remove`` operations; *replaced_or_removed_id* is the + previous step id for ``replace``/``remove`` operations, otherwise + ``None``. + """ + location = find_step(steps, edit.anchor) + if location is None: + raise ValueError(f"Anchor '{edit.anchor}' not found in workflow steps.") + + parent_list, index = location + old_step = parent_list[index] + old_step_id = old_step.get("id") if isinstance(old_step, dict) else None + + if edit.operation == "insert_after": + new_step = copy.deepcopy(edit.step) + parent_list.insert(index + 1, new_step) + step_id = str(new_step.get("id")) if isinstance(new_step, dict) else "" + return steps, ComposedStep(step_id, source), None + if edit.operation == "insert_before": + new_step = copy.deepcopy(edit.step) + parent_list.insert(index, new_step) + step_id = str(new_step.get("id")) if isinstance(new_step, dict) else "" + return steps, ComposedStep(step_id, source), None + if edit.operation == "replace": + new_step = copy.deepcopy(edit.step) + parent_list[index] = new_step + step_id = str(new_step.get("id")) if isinstance(new_step, dict) else "" + return steps, ComposedStep(step_id, source), old_step_id + if edit.operation == "remove": + del parent_list[index] + return steps, None, old_step_id + raise ValueError(f"Unsupported edit operation: {edit.operation}") + + +def _build_attribution( + steps: list[dict[str, Any]], + sources: dict[str, str], +) -> list[ComposedStep]: + """Build an ordered attribution list from the composed step tree.""" + result: list[ComposedStep] = [] + for step in steps: + if not isinstance(step, dict): + continue + step_id = step.get("id") + if isinstance(step_id, str): + result.append(ComposedStep(step_id, sources.get(step_id, "unknown"))) + for key in _NESTED_LIST_KEYS: + nested = step.get(key) + if isinstance(nested, list): + result.extend(_build_attribution(nested, sources)) + cases = step.get("cases") + if isinstance(cases, dict): + for case_steps in cases.values(): + if isinstance(case_steps, list): + result.extend(_build_attribution(case_steps, sources)) + return result + + +def merge_steps( + base_steps: list[dict[str, Any]], + overlays: list[OverlayLayer], +) -> tuple[list[dict[str, Any]], list[ComposedStep]]: + """Apply overlays to base steps in merge order and return composed steps. + + *overlays* is expected to be sorted by merge order (lowest priority first, + highest priority last). The returned step list is a deep copy of the base; + base_steps is never mutated. + + Higher-wins semantics are enforced for edits that target the same base + anchor: the highest-priority edit (last in *overlays*) decides the fate of + the anchor. A lower-priority ``remove`` cannot prevent a higher-priority + ``replace`` or ``insert_*`` on the same anchor. + """ + steps = copy.deepcopy(base_steps) + sources: dict[str, str] = {} + _init_sources_recursively(steps, sources) + + # Group edits by anchor, preserving merge order. + edits_by_anchor: dict[str, list[tuple[OverlayLayer, OverlayEdit]]] = {} + for layer in overlays: + for edit in layer.overlay.edits: + edits_by_anchor.setdefault(edit.anchor, []).append((layer, edit)) + + for anchor, edits in edits_by_anchor.items(): + # Highest-priority edit is last because *overlays* is already sorted. + _, winning_edit = edits[-1] + + if winning_edit.operation == "remove": + # Anchor is removed; ignore all other edits on this anchor. + location = find_step(steps, anchor) + if location is not None: + parent_list, index = location + removed_step = parent_list[index] + removed_id = removed_step.get("id") if isinstance(removed_step, dict) else None + del parent_list[index] + if isinstance(removed_id, str): + sources.pop(removed_id, None) + continue + + # For replace/insert_*, the anchor survives. Only the highest-priority + # replace is applied; lower-priority replaces on the same anchor are + # skipped. Inserts are applied in merge order. + if winning_edit.operation == "replace": + winning_layer, _ = edits[-1] + steps, composed, replaced_id = apply_edit(steps, winning_edit, winning_layer.source) + if isinstance(replaced_id, str): + sources.pop(replaced_id, None) + if composed is not None: + sources[composed.step_id] = composed.source + + for layer, edit in edits: + if edit.operation in ("insert_after", "insert_before"): + steps, composed, replaced_id = apply_edit(steps, edit, layer.source) + if replaced_id is not None: + sources.pop(replaced_id, None) + if composed is not None: + sources[composed.step_id] = composed.source + + attribution = _build_attribution(steps, sources) + return steps, attribution + + +def validate_edits( + edits: list[OverlayEdit], + base_step_ids: set[str], +) -> list[str]: + """Validate overlay edits against a set of known base step IDs. + + Returns a list of human-readable error messages. Does not raise. + """ + errors: list[str] = [] + for idx, edit in enumerate(edits): + if edit.operation not in VALID_OPERATIONS: + errors.append(f"Edit {idx}: invalid operation {edit.operation!r}.") + continue + if edit.anchor not in base_step_ids: + errors.append( + f"Edit {idx}: anchor '{edit.anchor}' does not match any base step id." + ) + if edit.operation == "remove": + if edit.step is not None: + errors.append(f"Edit {idx}: 'remove' must not include a step.") + continue + if not isinstance(edit.step, dict): + errors.append(f"Edit {idx}: '{edit.operation}' requires a step mapping.") + continue + step_id = edit.step.get("id") + if not isinstance(step_id, str) or not step_id: + errors.append(f"Edit {idx}: step is missing required 'id'.") + continue + if ":" in step_id: + errors.append( + f"Edit {idx}: step id {step_id!r} contains ':' which is reserved " + "for engine-generated nested IDs." + ) + return errors diff --git a/src/specify_cli/workflows/overlays/schema.py b/src/specify_cli/workflows/overlays/schema.py new file mode 100644 index 0000000000..5652d3f713 --- /dev/null +++ b/src/specify_cli/workflows/overlays/schema.py @@ -0,0 +1,174 @@ +"""Workflow overlay schema — dataclasses and validation for overlay manifests.""" + +from __future__ import annotations + +import re +from dataclasses import dataclass +from typing import Any, Literal + + +# 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"}) + +VALID_OPERATIONS = frozenset({"insert_after", "insert_before", "replace", "remove"}) + +# Map shorthand keys to operation names. +_SHORTHAND_OPERATION_KEYS: frozenset[str] = VALID_OPERATIONS + + +@dataclass(frozen=True) +class OverlayEdit: + """A single edit operation on a workflow step list.""" + + operation: Literal["insert_after", "insert_before", "replace", "remove"] + anchor: str + step: dict[str, Any] | None = None + + +@dataclass +class Overlay: + """A declared overlay (one YAML file).""" + + id: str + extends: str + priority: int + edits: list[OverlayEdit] + enabled: bool = True + + +def _validate_safe_id(value: str, field_name: str, allow_reserved: bool = False) -> str | None: + """Return an error message if *value* is not a safe path segment ID.""" + if not isinstance(value, str) or not value: + return f"Overlay '{field_name}' is required and must be a non-empty string." + if not _SAFE_ID_PATTERN.match(value): + return ( + f"Overlay '{field_name}' {value!r} contains invalid characters; " + "only lowercase letters, digits, and hyphens are allowed." + ) + if not allow_reserved and value in _RESERVED_OVERLAY_WORKFLOW_IDS: + return f"Overlay '{field_name}' {value!r} is reserved." + return None + + +def _parse_edit(edit_raw: dict[str, Any], idx: int) -> tuple[OverlayEdit | None, str | None]: + """Parse a single edit dict into an OverlayEdit or an error string.""" + shorthand_keys = [key for key in _SHORTHAND_OPERATION_KEYS if key in edit_raw] + has_operation = "operation" in edit_raw + + operation: str | None = None + anchor: Any = None + + if shorthand_keys and has_operation: + return None, ( + f"Edit at index {idx} mixes shorthand operation key " + f"({shorthand_keys[0]!r}) with explicit 'operation' field." + ) + + if len(shorthand_keys) > 1: + return None, ( + f"Edit at index {idx} has multiple operation keys: " + f"{', '.join(repr(k) for k in shorthand_keys)}." + ) + + if shorthand_keys: + operation = shorthand_keys[0] + anchor = edit_raw[operation] + elif has_operation: + operation = edit_raw.get("operation") + anchor = edit_raw.get("anchor") + else: + return None, f"Edit at index {idx} has no operation; expected one of {sorted(VALID_OPERATIONS)}." + + if operation not in VALID_OPERATIONS: + return None, f"Edit at index {idx} has invalid operation {operation!r}." + + if not isinstance(anchor, str) or not anchor: + return None, f"Edit at index {idx} has invalid 'anchor'." + + step = edit_raw.get("step") + if operation == "remove": + if step is not None: + return None, f"Edit at index {idx} ('remove') must not include 'step'." + return OverlayEdit(operation=operation, anchor=anchor), None + + if not isinstance(step, dict): + return None, f"Edit at index {idx} ('{operation}') requires 'step' mapping." + step_id = step.get("id") + if not isinstance(step_id, str) or not step_id: + return None, f"Edit at index {idx} step is missing required 'id'." + if ":" in step_id: + return None, ( + f"Edit at index {idx} step id {step_id!r} contains ':' " + "which is reserved for engine-generated nested IDs." + ) + return OverlayEdit(operation=operation, anchor=anchor, step=step), None + + +def validate_overlay_yaml(data: dict[str, Any]) -> tuple[Overlay | None, list[str]]: + """Validate an overlay manifest dict and return (Overlay, errors). + + Errors are returned as a list of strings; validation never raises. + """ + errors: list[str] = [] + + if not isinstance(data, dict): + return None, ["Overlay manifest must be a mapping."] + + overlay_id = data.get("id") + if err := _validate_safe_id(overlay_id, "id"): + errors.append(err) + overlay_id = "" + + extends = data.get("extends") + if err := _validate_safe_id(extends, "extends"): + errors.append(err) + extends = "" + + priority = data.get("priority") + if isinstance(priority, bool) or not isinstance(priority, int): + errors.append( + f"Overlay 'priority' must be an integer >= 1, got {priority!r}." + ) + priority = 0 + elif priority < 1: + errors.append( + f"Overlay 'priority' must be an integer >= 1, got {priority!r}." + ) + + edits_raw = data.get("edits") + edits: list[OverlayEdit] = [] + if not isinstance(edits_raw, list): + errors.append("Overlay 'edits' is required and must be a list.") + elif not edits_raw: + errors.append("Overlay 'edits' must be a non-empty list.") + else: + for idx, edit_raw in enumerate(edits_raw): + if not isinstance(edit_raw, dict): + errors.append(f"Edit at index {idx} must be a mapping.") + continue + edit, err = _parse_edit(edit_raw, idx) + if err: + errors.append(err) + continue + if edit is not None: + edits.append(edit) + + enabled = data.get("enabled", True) + if not isinstance(enabled, bool): + errors.append("Overlay 'enabled' must be a boolean.") + enabled = bool(enabled) + + if errors: + return None, errors + + return ( + Overlay( + id=overlay_id, + extends=extends, + priority=priority, + edits=edits, + enabled=enabled, + ), + [], + ) diff --git a/tests/workflows/conftest.py b/tests/workflows/conftest.py new file mode 100644 index 0000000000..03d28890dd --- /dev/null +++ b/tests/workflows/conftest.py @@ -0,0 +1,26 @@ +"""Shared fixtures for workflow tests.""" + +from __future__ import annotations + +import shutil +import sys +import tempfile +from pathlib import Path + +import pytest + + +@pytest.fixture +def temp_dir(): + """Create a temporary directory for tests.""" + tmpdir = tempfile.mkdtemp() + yield Path(tmpdir) + shutil.rmtree(tmpdir, ignore_errors=(sys.platform == "win32")) + + +@pytest.fixture +def project_dir(temp_dir): + """Create a mock spec-kit project with ``.specify/workflows/`` directory.""" + workflows_dir = temp_dir / ".specify" / "workflows" + workflows_dir.mkdir(parents=True, exist_ok=True) + return temp_dir diff --git a/tests/workflows/test_overlay_commands.py b/tests/workflows/test_overlay_commands.py new file mode 100644 index 0000000000..3e4aad9131 --- /dev/null +++ b/tests/workflows/test_overlay_commands.py @@ -0,0 +1,314 @@ +"""Tests for workflow overlay CLI commands.""" + +from __future__ import annotations + +from pathlib import Path + +import pytest +import yaml +from typer.testing import CliRunner + +from specify_cli import app + + +runner = CliRunner() + + +@pytest.fixture +def project_dir(tmp_path): + """Create a mock spec-kit project with ``.specify/workflows/`` directory.""" + workflows_dir = tmp_path / ".specify" / "workflows" + workflows_dir.mkdir(parents=True, exist_ok=True) + return tmp_path + + +def _write_workflow(project_root: Path, workflow_id: str, data: dict) -> Path: + wf_dir = project_root / ".specify" / "workflows" / workflow_id + wf_dir.mkdir(parents=True, exist_ok=True) + wf_path = wf_dir / "workflow.yml" + wf_path.write_text(yaml.safe_dump(data), encoding="utf-8") + return wf_path + + +def _write_overlay(project_root: Path, workflow_id: str, overlay_id: str, data: dict) -> Path: + ov_dir = project_root / ".specify" / "workflows" / "overlays" / workflow_id + ov_dir.mkdir(parents=True, exist_ok=True) + ov_path = ov_dir / f"{overlay_id}.yml" + ov_path.write_text(yaml.safe_dump(data), encoding="utf-8") + return ov_path + + +class TestOverlayCli: + """CLI-level tests for ``specify workflow overlay *``.""" + + def test_overlay_add(self, project_dir, monkeypatch): + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + _write_workflow( + project_dir, + "wf", + { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "echo"}], + }, + ) + overlay_file = project_dir / "overlay.yml" + overlay_file.write_text( + yaml.safe_dump( + { + "id": "ov1", + "extends": "wf", + "priority": 10, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "new", "type": "command", "command": "echo"}, + } + ], + } + ), + encoding="utf-8", + ) + + result = runner.invoke( + app, ["workflow", "overlay", "add", str(overlay_file), "--priority", "5"] + ) + assert result.exit_code == 0, result.output + assert "Overlay 'ov1' added" in result.output + + installed = project_dir / ".specify" / "workflows" / "overlays" / "wf" / "ov1.yml" + assert installed.is_file() + data = yaml.safe_load(installed.read_text(encoding="utf-8")) + assert data["priority"] == 5 + + def test_overlay_set_priority(self, project_dir, monkeypatch): + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + _write_workflow( + project_dir, + "wf", + { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "echo"}], + }, + ) + _write_overlay( + project_dir, + "wf", + "ov1", + { + "id": "ov1", + "extends": "wf", + "priority": 10, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "new", "type": "command", "command": "echo"}, + } + ], + }, + ) + + result = runner.invoke( + app, ["workflow", "overlay", "set-priority", "wf", "ov1", "20"] + ) + assert result.exit_code == 0, result.output + data = yaml.safe_load( + ( + project_dir / ".specify" / "workflows" / "overlays" / "wf" / "ov1.yml" + ).read_text(encoding="utf-8") + ) + assert data["priority"] == 20 + + def test_overlay_disable_and_enable(self, project_dir, monkeypatch): + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + _write_workflow( + project_dir, + "wf", + { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "echo"}], + }, + ) + _write_overlay( + project_dir, + "wf", + "ov1", + { + "id": "ov1", + "extends": "wf", + "priority": 10, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "new", "type": "command", "command": "echo"}, + } + ], + }, + ) + + result = runner.invoke(app, ["workflow", "overlay", "disable", "wf", "ov1"]) + assert result.exit_code == 0, result.output + data = yaml.safe_load( + ( + project_dir / ".specify" / "workflows" / "overlays" / "wf" / "ov1.yml" + ).read_text(encoding="utf-8") + ) + assert data["enabled"] is False + + result = runner.invoke(app, ["workflow", "overlay", "enable", "wf", "ov1"]) + assert result.exit_code == 0, result.output + data = yaml.safe_load( + ( + project_dir / ".specify" / "workflows" / "overlays" / "wf" / "ov1.yml" + ).read_text(encoding="utf-8") + ) + assert data["enabled"] is True + + def test_overlay_remove(self, project_dir, monkeypatch): + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + _write_workflow( + project_dir, + "wf", + { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "echo"}], + }, + ) + _write_overlay( + project_dir, + "wf", + "ov1", + { + "id": "ov1", + "extends": "wf", + "priority": 10, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "new", "type": "command", "command": "echo"}, + } + ], + }, + ) + + result = runner.invoke(app, ["workflow", "overlay", "remove", "wf", "ov1"]) + assert result.exit_code == 0, result.output + assert not ( + project_dir / ".specify" / "workflows" / "overlays" / "wf" / "ov1.yml" + ).exists() + + def test_overlay_list(self, project_dir, monkeypatch): + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + _write_workflow( + project_dir, + "wf", + { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "echo"}], + }, + ) + _write_overlay( + project_dir, + "wf", + "ov1", + { + "id": "ov1", + "extends": "wf", + "priority": 10, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "new", "type": "command", "command": "echo"}, + } + ], + }, + ) + + result = runner.invoke(app, ["workflow", "overlay", "list", "wf"]) + assert result.exit_code == 0, result.output + assert "ov1" in result.output + + def test_workflow_resolve(self, project_dir, monkeypatch): + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + _write_workflow( + project_dir, + "wf", + { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "echo"}], + }, + ) + _write_overlay( + project_dir, + "wf", + "ov1", + { + "id": "ov1", + "extends": "wf", + "priority": 10, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "new", "type": "command", "command": "echo"}, + } + ], + }, + ) + + result = runner.invoke(app, ["workflow", "resolve", "wf"]) + assert result.exit_code == 0, result.output + assert "base" in result.output + assert "project:ov1" in result.output + assert "new" in result.output + + def test_workflow_add_copies_overlays(self, project_dir, monkeypatch, tmp_path): + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + source_dir = tmp_path / "source-wf" + source_dir.mkdir() + (source_dir / "workflow.yml").write_text( + yaml.safe_dump( + { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "echo"}], + } + ), + encoding="utf-8", + ) + overlays_dir = source_dir / "overlays" + overlays_dir.mkdir() + (overlays_dir / "ov1.yml").write_text( + yaml.safe_dump( + { + "id": "ov1", + "extends": "wf", + "priority": 10, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "new", "type": "command", "command": "echo"}, + } + ], + } + ), + encoding="utf-8", + ) + + result = runner.invoke(app, ["workflow", "add", str(source_dir)]) + assert result.exit_code == 0, result.output + installed_overlay = ( + project_dir / ".specify" / "workflows" / "wf" / "overlays" / "ov1.yml" + ) + assert installed_overlay.is_file() diff --git a/tests/workflows/test_overlay_composer.py b/tests/workflows/test_overlay_composer.py new file mode 100644 index 0000000000..dc3b0e90d6 --- /dev/null +++ b/tests/workflows/test_overlay_composer.py @@ -0,0 +1,106 @@ +"""Tests for StepListComposer validation and error handling.""" + +from __future__ import annotations + +from pathlib import Path + +import pytest +import yaml + +from specify_cli.workflows.overlays import WorkflowResolver +from specify_cli.workflows.overlays.composer import StepListComposer +from specify_cli.workflows.overlays.layer_sources import BaseWorkflowSource, Layer +from specify_cli.workflows.overlays.schema import Overlay, OverlayEdit + + +def _write_workflow(project_root: Path, workflow_id: str, data: dict) -> Path: + wf_dir = project_root / ".specify" / "workflows" / workflow_id + wf_dir.mkdir(parents=True, exist_ok=True) + wf_path = wf_dir / "workflow.yml" + wf_path.write_text(yaml.safe_dump(data), encoding="utf-8") + return wf_path + + +class TestStepListComposerValidation: + """Composer validates edits before applying them.""" + + def test_composer_reports_invalid_anchors(self, project_dir): + _write_workflow( + project_dir, + "wf", + { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "echo"}], + }, + ) + base_layer = BaseWorkflowSource(project_dir).collect("wf")[0] + + overlay = Overlay( + id="ov", + extends="wf", + priority=10, + edits=[OverlayEdit("insert_after", "missing", {"id": "new", "type": "command", "command": "echo"})], + ) + layer = Layer(content=overlay, source="project:ov", tier="project-overlay", priority=10) + composer = StepListComposer() + with pytest.raises(ValueError, match="does not match any base step id"): + composer.compose([base_layer, layer]) + + def test_composer_validates_edits_before_merge(self, project_dir): + _write_workflow( + project_dir, + "wf", + { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "echo"}], + }, + ) + overlay = Overlay( + id="ov", + extends="wf", + priority=10, + edits=[OverlayEdit("replace", "a", {"id": "bad:id", "type": "command", "command": "echo"})], + ) + layer = Layer(content=overlay, source="project:ov", tier="project-overlay", priority=10) + composer = StepListComposer() + with pytest.raises(ValueError, match="bad:id"): + composer.compose([BaseWorkflowSource(project_dir).collect("wf")[0], layer]) + + def test_resolver_reports_invalid_anchor_as_validation_error(self, project_dir): + _write_workflow( + project_dir, + "wf", + { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "echo"}], + }, + ) + overlay_file = project_dir / "overlay.yml" + overlay_file.write_text( + yaml.safe_dump( + { + "id": "ov", + "extends": "wf", + "priority": 10, + "edits": [ + { + "operation": "insert_after", + "anchor": "missing", + "step": {"id": "new", "type": "command", "command": "echo"}, + } + ], + } + ), + encoding="utf-8", + ) + # Manually inject the overlay by writing it to disk in the correct location. + overlay_dir = project_dir / ".specify" / "workflows" / "overlays" / "wf" + overlay_dir.mkdir(parents=True, exist_ok=True) + (overlay_dir / "ov.yml").write_text(overlay_file.read_text(encoding="utf-8"), encoding="utf-8") + + resolver = WorkflowResolver(project_dir) + with pytest.raises(ValueError, match="missing"): + resolver.resolve("wf") diff --git a/tests/workflows/test_overlay_merge.py b/tests/workflows/test_overlay_merge.py new file mode 100644 index 0000000000..01be81bf48 --- /dev/null +++ b/tests/workflows/test_overlay_merge.py @@ -0,0 +1,402 @@ +"""Tests for the workflow overlay merge engine.""" + +from __future__ import annotations + +import copy +from typing import Any + +import pytest + +from specify_cli.workflows.overlays.merge import ( + ComposedStep, + OverlayLayer, + apply_edit, + find_step, + merge_steps, + validate_edits, +) +from specify_cli.workflows.overlays.schema import Overlay, OverlayEdit + + +def _step(id: str, **kwargs: Any) -> dict[str, Any]: # noqa: A002 + """Build a minimal step dict with the given id.""" + return {"id": id, "type": "command", "command": "speckit.specify", **kwargs} + + +def _layer(overlay: Overlay, source: str) -> OverlayLayer: + """Build an OverlayLayer for merge_steps.""" + return OverlayLayer(overlay, source) + + +class TestFindStep: + """Recursive anchor lookup across nested step lists.""" + + def test_find_step_flat(self): + steps = [_step("a"), _step("b"), _step("c")] + result = find_step(steps, "b") + assert result is not None + assert result[0] is steps + assert result[1] == 1 + + def test_find_step_missing(self): + steps = [_step("a"), _step("b")] + assert find_step(steps, "missing") is None + + def test_find_step_in_then(self): + steps = [ + { + "id": "if-1", + "type": "if", + "condition": "true", + "then": [_step("then-a")], + "else": [_step("else-b")], + }, + ] + result = find_step(steps, "then-a") + assert result is not None + assert result[0] is steps[0]["then"] + assert result[1] == 0 + + def test_find_step_in_else(self): + steps = [ + { + "id": "if-1", + "type": "if", + "condition": "true", + "then": [_step("then-a")], + "else": [_step("else-b")], + }, + ] + result = find_step(steps, "else-b") + assert result is not None + assert result[0] is steps[0]["else"] + assert result[1] == 0 + + def test_find_step_in_nested_steps(self): + steps = [ + { + "id": "while-1", + "type": "while", + "condition": "true", + "steps": [_step("inner-a"), _step("inner-b")], + }, + ] + result = find_step(steps, "inner-b") + assert result is not None + assert result[0] is steps[0]["steps"] + assert result[1] == 1 + + def test_find_step_in_switch_cases(self): + steps = [ + { + "id": "switch-1", + "type": "switch", + "expression": "{{ inputs.x }}", + "cases": { + "one": [_step("case-a")], + "two": [_step("case-b")], + }, + "default": [_step("default-c")], + }, + ] + assert find_step(steps, "case-b")[1] == 0 + assert find_step(steps, "default-c")[1] == 0 + + def test_find_step_not_in_fan_out_template(self): + steps = [ + { + "id": "fan-1", + "type": "fan-out", + "items": "{{ inputs.items }}", + "step": {"id": "template-x", "type": "command", "command": "echo"}, + }, + ] + assert find_step(steps, "template-x") is None + + +class TestApplyEdit: + """Single edit operations against a step list.""" + + def test_insert_after(self): + steps = [_step("a"), _step("b")] + new_step = _step("new") + result, composed, replaced = apply_edit(steps, OverlayEdit("insert_after", "a", new_step), "project") + assert [s["id"] for s in result] == ["a", "new", "b"] + assert composed == ComposedStep("new", "project") + assert replaced is None + + def test_insert_before(self): + steps = [_step("a"), _step("b")] + new_step = _step("new") + result, composed, replaced = apply_edit(steps, OverlayEdit("insert_before", "b", new_step), "project") + assert [s["id"] for s in result] == ["a", "new", "b"] + assert replaced is None + + def test_replace(self): + steps = [_step("a"), _step("b")] + new_step = _step("replaced") + result, composed, replaced = apply_edit(steps, OverlayEdit("replace", "b", new_step), "installed:x") + assert [s["id"] for s in result] == ["a", "replaced"] + assert composed == ComposedStep("replaced", "installed:x") + assert replaced == "b" + + def test_remove(self): + steps = [_step("a"), _step("b"), _step("c")] + result, composed, replaced = apply_edit(steps, OverlayEdit("remove", "b"), "project") + assert [s["id"] for s in result] == ["a", "c"] + assert composed is None + assert replaced == "b" + + def test_apply_edit_anchors_nested(self): + steps = [ + { + "id": "if-1", + "type": "if", + "condition": "true", + "then": [_step("then-a")], + }, + ] + new_step = _step("new") + result, _, _ = apply_edit(steps, OverlayEdit("insert_after", "then-a", new_step), "project") + assert [s["id"] for s in result[0]["then"]] == ["then-a", "new"] + + def test_apply_edit_missing_anchor(self): + steps = [_step("a")] + new_step = _step("new") + with pytest.raises(ValueError, match="Anchor 'missing' not found"): + apply_edit(steps, OverlayEdit("insert_after", "missing", new_step), "project") + + +class TestMergeSteps: + """Composition of multiple overlays in merge order.""" + + def test_merge_steps_no_overlays(self): + base = [_step("a"), _step("b")] + steps, attribution = merge_steps(base, []) + assert [s["id"] for s in steps] == ["a", "b"] + assert attribution == [ComposedStep("a", "base"), ComposedStep("b", "base")] + + def test_merge_steps_single_overlay(self): + base = [_step("a"), _step("b")] + overlay = Overlay( + id="ov1", + extends="wf", + priority=10, + edits=[OverlayEdit("insert_after", "a", _step("new"))], + ) + steps, attribution = merge_steps(base, [_layer(overlay, "project:ov1")]) + assert [s["id"] for s in steps] == ["a", "new", "b"] + assert attribution == [ + ComposedStep("a", "base"), + ComposedStep("new", "project:ov1"), + ComposedStep("b", "base"), + ] + + def test_merge_steps_higher_priority_wins(self): + base = [_step("a")] + low = Overlay( + id="low", + extends="wf", + priority=5, + edits=[OverlayEdit("insert_after", "a", _step("low-step"))], + ) + high = Overlay( + id="high", + extends="wf", + priority=10, + edits=[OverlayEdit("insert_after", "a", _step("high-step"))], + ) + steps, attribution = merge_steps(base, [_layer(low, "project:low"), _layer(high, "project:high")]) + # low applied first, then high; both insert after 'a', so high-step ends + # closer to the anchor (higher priority wins the conflict). + assert [s["id"] for s in steps] == ["a", "high-step", "low-step"] + assert attribution == [ + ComposedStep("a", "base"), + ComposedStep("high-step", "project:high"), + ComposedStep("low-step", "project:low"), + ] + + def test_merge_steps_replace_wins_over_insert(self): + base = [_step("a")] + insert = Overlay( + id="insert", + extends="wf", + priority=5, + edits=[OverlayEdit("insert_after", "a", _step("inserted"))], + ) + replace = Overlay( + id="replace", + extends="wf", + priority=10, + edits=[OverlayEdit("replace", "inserted", _step("replaced"))], + ) + steps, _ = merge_steps(base, [_layer(insert, "project:insert"), _layer(replace, "project:replace")]) + assert [s["id"] for s in steps] == ["a", "replaced"] + + def test_merge_steps_does_not_mutate_base(self): + base = [_step("a")] + overlay = Overlay( + id="ov1", + extends="wf", + priority=10, + edits=[OverlayEdit("insert_after", "a", _step("new"))], + ) + original = copy.deepcopy(base) + merge_steps(base, [_layer(overlay, "project:ov1")]) + assert base == original + + def test_merge_steps_attribution_uses_source_not_overlay_id(self): + base = [_step("a")] + overlay = Overlay( + id="same-id", + extends="wf", + priority=10, + edits=[OverlayEdit("insert_after", "a", _step("new"))], + ) + steps, attribution = merge_steps(base, [_layer(overlay, "installed:same-id")]) + assert [s["id"] for s in steps] == ["a", "new"] + assert attribution == [ + ComposedStep("a", "base"), + ComposedStep("new", "installed:same-id"), + ] + + def test_merge_steps_nested_base_attribution(self): + base = [ + { + "id": "if-1", + "type": "if", + "condition": "true", + "then": [_step("then-a")], + "else": [_step("else-b")], + }, + ] + steps, attribution = merge_steps(base, []) + assert attribution == [ + ComposedStep("if-1", "base"), + ComposedStep("then-a", "base"), + ComposedStep("else-b", "base"), + ] + + def test_merge_steps_higher_replace_wins_lower_replace_same_anchor(self): + base = [_step("implement")] + low = Overlay( + id="low", + extends="wf", + priority=5, + edits=[OverlayEdit("replace", "implement", _step("low-implement"))], + ) + high = Overlay( + id="high", + extends="wf", + priority=10, + edits=[OverlayEdit("replace", "implement", _step("high-implement"))], + ) + steps, attribution = merge_steps(base, [_layer(low, "project:low"), _layer(high, "project:high")]) + assert [s["id"] for s in steps] == ["high-implement"] + assert any( + composed.step_id == "high-implement" and composed.source == "project:high" + for composed in attribution + ) + + def test_merge_steps_higher_replace_wins_after_lower_remove_same_anchor(self): + base = [_step("implement")] + low = Overlay( + id="low", + extends="wf", + priority=5, + edits=[OverlayEdit("remove", "implement")], + ) + high = Overlay( + id="high", + extends="wf", + priority=10, + edits=[OverlayEdit("replace", "implement", _step("high-implement"))], + ) + steps, attribution = merge_steps(base, [_layer(low, "project:low"), _layer(high, "project:high")]) + assert [s["id"] for s in steps] == ["high-implement"] + + def test_merge_steps_higher_insert_wins_after_lower_remove_same_anchor(self): + base = [_step("implement")] + low = Overlay( + id="low", + extends="wf", + priority=5, + edits=[OverlayEdit("remove", "implement")], + ) + high = Overlay( + id="high", + extends="wf", + priority=10, + edits=[OverlayEdit("insert_after", "implement", _step("high-after"))], + ) + steps, attribution = merge_steps(base, [_layer(low, "project:low"), _layer(high, "project:high")]) + assert [s["id"] for s in steps] == ["implement", "high-after"] + assert attribution == [ + ComposedStep("implement", "base"), + ComposedStep("high-after", "project:high"), + ] + + def test_merge_steps_project_wins_tie_over_installed_same_anchor(self): + base = [_step("a")] + installed = Overlay( + id="same", + extends="wf", + priority=10, + edits=[OverlayEdit("replace", "a", _step("installed-replace"))], + ) + project = Overlay( + id="same", + extends="wf", + priority=10, + edits=[OverlayEdit("replace", "a", _step("project-replace"))], + ) + # Merge order: installed first, then project (project wins tie). + steps, attribution = merge_steps( + base, + [ + _layer(installed, "installed:same"), + _layer(project, "project:same"), + ], + ) + assert [s["id"] for s in steps] == ["project-replace"] + assert any( + composed.step_id == "project-replace" and composed.source == "project:same" + for composed in attribution + ) + + def test_merge_steps_unknown_anchor_still_raises(self): + base = [_step("a")] + overlay = Overlay( + id="ov", + extends="wf", + priority=10, + edits=[OverlayEdit("replace", "missing", _step("new"))], + ) + with pytest.raises(ValueError, match="Anchor 'missing' not found"): + merge_steps(base, [_layer(overlay, "project:ov")]) + + +class TestValidateEdits: + """Edit validation against known base step IDs.""" + + def test_valid_edits(self): + edits = [ + OverlayEdit("insert_after", "a", _step("new")), + OverlayEdit("remove", "b"), + ] + assert validate_edits(edits, {"a", "b"}) == [] + + def test_invalid_anchor(self): + edits = [OverlayEdit("insert_after", "missing", _step("new"))] + errors = validate_edits(edits, {"a"}) + assert any("missing" in e for e in errors) + + def test_step_id_contains_colon(self): + edits = [OverlayEdit("insert_after", "a", _step("bad:id"))] + errors = validate_edits(edits, {"a"}) + assert any("':'" in e for e in errors) + + def test_remove_requires_no_step(self): + edits = [OverlayEdit("remove", "a", _step("extra"))] + errors = validate_edits(edits, {"a"}) + assert len(errors) > 0 diff --git a/tests/workflows/test_overlay_schema.py b/tests/workflows/test_overlay_schema.py new file mode 100644 index 0000000000..72595b6d22 --- /dev/null +++ b/tests/workflows/test_overlay_schema.py @@ -0,0 +1,218 @@ +"""Tests for overlay YAML schema normalization, especially shorthand edits.""" + +from __future__ import annotations + +import pytest + +from specify_cli.workflows.overlays.schema import ( + OverlayEdit, + validate_overlay_yaml, +) + + +class TestShorthandEdits: + """Requirements-compliant shorthand edit format.""" + + def test_shorthand_insert_after(self): + overlay, errors = validate_overlay_yaml( + { + "id": "lint", + "extends": "wf", + "priority": 10, + "edits": [ + { + "insert_after": "implement", + "step": {"id": "lint", "type": "shell", "command": "npm run lint"}, + } + ], + } + ) + assert not errors, errors + assert overlay is not None + assert overlay.edits == [ + OverlayEdit("insert_after", "implement", {"id": "lint", "type": "shell", "command": "npm run lint"}) + ] + + def test_shorthand_insert_before(self): + overlay, errors = validate_overlay_yaml( + { + "id": "ov", + "extends": "wf", + "priority": 10, + "edits": [ + { + "insert_before": "a", + "step": {"id": "b", "type": "command", "command": "echo"}, + } + ], + } + ) + assert not errors, errors + assert overlay is not None + assert overlay.edits == [ + OverlayEdit("insert_before", "a", {"id": "b", "type": "command", "command": "echo"}) + ] + + def test_shorthand_replace(self): + overlay, errors = validate_overlay_yaml( + { + "id": "ov", + "extends": "wf", + "priority": 10, + "edits": [ + { + "replace": "a", + "step": {"id": "a", "type": "command", "command": "echo"}, + } + ], + } + ) + assert not errors, errors + assert overlay is not None + assert overlay.edits == [ + OverlayEdit("replace", "a", {"id": "a", "type": "command", "command": "echo"}) + ] + + def test_shorthand_remove(self): + overlay, errors = validate_overlay_yaml( + { + "id": "ov", + "extends": "wf", + "priority": 10, + "edits": [{"remove": "a"}], + } + ) + assert not errors, errors + assert overlay is not None + assert overlay.edits == [OverlayEdit("remove", "a")] + + def test_explicit_operation_format_still_valid(self): + overlay, errors = validate_overlay_yaml( + { + "id": "ov", + "extends": "wf", + "priority": 10, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "b", "type": "command", "command": "echo"}, + } + ], + } + ) + assert not errors, errors + assert overlay is not None + assert overlay.edits == [ + OverlayEdit("insert_after", "a", {"id": "b", "type": "command", "command": "echo"}) + ] + + def test_multiple_operation_fields_rejected(self): + overlay, errors = validate_overlay_yaml( + { + "id": "ov", + "extends": "wf", + "priority": 10, + "edits": [ + { + "insert_after": "a", + "remove": "a", + } + ], + } + ) + assert overlay is None + assert any("multiple" in e.lower() for e in errors), errors + + def test_invalid_operation_field_rejected(self): + overlay, errors = validate_overlay_yaml( + { + "id": "ov", + "extends": "wf", + "priority": 10, + "edits": [{"destroy": "a"}], + } + ) + assert overlay is None + assert any("operation" in e.lower() for e in errors), errors + + def test_shorthand_and_explicit_mixed_list(self): + overlay, errors = validate_overlay_yaml( + { + "id": "ov", + "extends": "wf", + "priority": 10, + "edits": [ + {"insert_after": "a", "step": {"id": "b", "type": "command", "command": "echo"}}, + { + "operation": "remove", + "anchor": "c", + }, + ], + } + ) + assert not errors, errors + assert overlay is not None + assert overlay.edits == [ + OverlayEdit("insert_after", "a", {"id": "b", "type": "command", "command": "echo"}), + OverlayEdit("remove", "c"), + ] + + def test_shorthand_remove_must_not_include_step(self): + overlay, errors = validate_overlay_yaml( + { + "id": "ov", + "extends": "wf", + "priority": 10, + "edits": [ + { + "remove": "a", + "step": {"id": "b", "type": "command", "command": "echo"}, + } + ], + } + ) + assert overlay is None + assert any("remove" in e.lower() and "step" in e.lower() for e in errors), errors + + +class TestOverlayIdValidation: + """Overlay and workflow IDs must be safe path segments.""" + + @pytest.mark.parametrize("overlay_id", ["../ov", "a/b", "a\\\\b", ".", "..", ""]) + def test_invalid_overlay_id_rejected(self, overlay_id): + overlay, errors = validate_overlay_yaml( + { + "id": overlay_id, + "extends": "wf", + "priority": 10, + "edits": [{"remove": "a"}], + } + ) + assert overlay is None + assert any("id" in e.lower() for e in errors), errors + + @pytest.mark.parametrize("extends", ["../wf", "a/b", "a\\\\b", ".", "..", ""]) + def test_invalid_extends_rejected(self, extends): + overlay, errors = validate_overlay_yaml( + { + "id": "ov", + "extends": extends, + "priority": 10, + "edits": [{"remove": "a"}], + } + ) + assert overlay is None + assert any("extends" in e.lower() for e in errors), errors + + def test_valid_dashed_id_accepted(self): + overlay, errors = validate_overlay_yaml( + { + "id": "my-overlay", + "extends": "my-workflow", + "priority": 10, + "edits": [{"remove": "a"}], + } + ) + assert not errors, errors + assert overlay is not None diff --git a/tests/workflows/test_overlay_security.py b/tests/workflows/test_overlay_security.py new file mode 100644 index 0000000000..3871d89217 --- /dev/null +++ b/tests/workflows/test_overlay_security.py @@ -0,0 +1,204 @@ +"""Security tests for workflow overlay path handling.""" + +from __future__ import annotations + +from pathlib import Path + +import pytest +import yaml +from typer.testing import CliRunner + +from specify_cli import app + + +runner = CliRunner() + + +@pytest.fixture +def project_dir(tmp_path): + """Create a mock spec-kit project with ``.specify/workflows/`` directory.""" + workflows_dir = tmp_path / ".specify" / "workflows" + workflows_dir.mkdir(parents=True, exist_ok=True) + return tmp_path + + +def _write_workflow(project_root: Path, workflow_id: str, data: dict) -> Path: + wf_dir = project_root / ".specify" / "workflows" / workflow_id + wf_dir.mkdir(parents=True, exist_ok=True) + wf_path = wf_dir / "workflow.yml" + wf_path.write_text(yaml.safe_dump(data), encoding="utf-8") + return wf_path + + +def _write_overlay(project_root: Path, workflow_id: str, overlay_id: str, data: dict) -> Path: + ov_dir = project_root / ".specify" / "workflows" / "overlays" / workflow_id + ov_dir.mkdir(parents=True, exist_ok=True) + ov_path = ov_dir / f"{overlay_id}.yml" + ov_path.write_text(yaml.safe_dump(data), encoding="utf-8") + return ov_path + + +class TestOverlayPathTraversal: + """Overlay CLI must stay inside the overlay directory.""" + + def test_overlay_add_rejects_traversal_in_workflow_id(self, project_dir, monkeypatch): + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + overlay_file = project_dir / "overlay.yml" + overlay_file.write_text( + yaml.safe_dump( + { + "id": "ov1", + "extends": "../wf", + "priority": 10, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "new", "type": "command", "command": "echo"}, + } + ], + } + ), + encoding="utf-8", + ) + + result = runner.invoke( + app, ["workflow", "overlay", "add", str(overlay_file), "--priority", "5"] + ) + assert result.exit_code != 0, result.output + assert "invalid" in result.output.lower() or "traversal" in result.output.lower() + + def test_overlay_add_rejects_traversal_in_overlay_id(self, project_dir, monkeypatch): + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + _write_workflow( + project_dir, + "wf", + { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "echo"}], + }, + ) + overlay_file = project_dir / "overlay.yml" + overlay_file.write_text( + yaml.safe_dump( + { + "id": "../../ov1", + "extends": "wf", + "priority": 10, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "new", "type": "command", "command": "echo"}, + } + ], + } + ), + encoding="utf-8", + ) + + result = runner.invoke( + app, ["workflow", "overlay", "add", str(overlay_file), "--priority", "5"] + ) + assert result.exit_code != 0, result.output + assert "invalid" in result.output.lower() or "traversal" in result.output.lower() + + def test_overlay_remove_cannot_escape_overlays_dir(self, project_dir, monkeypatch): + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + _write_workflow( + project_dir, + "wf", + { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "echo"}], + }, + ) + # Create a base workflow file that would be the traversal target. + target = project_dir / ".specify" / "workflows" / "wf" / "workflow.yml" + assert target.is_file() + + result = runner.invoke( + app, ["workflow", "overlay", "remove", "wf", "../wf/workflow"] + ) + assert result.exit_code != 0, result.output + assert target.is_file() + assert "Invalid" in result.output or "traversal" in result.output.lower() + + def test_overlay_remove_rejects_symlink(self, project_dir, monkeypatch): + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + _write_workflow( + project_dir, + "wf", + { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "echo"}], + }, + ) + _write_overlay( + project_dir, + "wf", + "ov1", + { + "id": "ov1", + "extends": "wf", + "priority": 10, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "new", "type": "command", "command": "echo"}, + } + ], + }, + ) + + overlay_dir = project_dir / ".specify" / "workflows" / "overlays" / "wf" + real_file = overlay_dir / "ov1.yml" + symlink_file = overlay_dir / "symlink.yml" + symlink_file.symlink_to(real_file) + + result = runner.invoke(app, ["workflow", "overlay", "remove", "wf", "symlink"]) + assert result.exit_code != 0, result.output + assert real_file.is_file() + assert "symlink" in result.output.lower() or "Invalid" in result.output + + def test_overlay_operations_reject_overlays_as_workflow_id(self, project_dir, monkeypatch): + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + result = runner.invoke(app, ["workflow", "overlay", "list", "overlays"]) + assert result.exit_code != 0, result.output + assert "Invalid" in result.output or "reserved" in result.output.lower() + + def test_overlay_set_priority_rejects_traversal(self, project_dir, monkeypatch): + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + _write_workflow( + project_dir, + "wf", + { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "echo"}], + }, + ) + result = runner.invoke( + app, ["workflow", "overlay", "set-priority", "wf", "../other", "10"] + ) + assert result.exit_code != 0, result.output + assert "invalid" in result.output.lower() or "traversal" in result.output.lower() + + def test_overlay_enable_rejects_traversal(self, project_dir, monkeypatch): + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + _write_workflow( + project_dir, + "wf", + { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "echo"}], + }, + ) + result = runner.invoke(app, ["workflow", "overlay", "enable", "wf", "../other"]) + assert result.exit_code != 0, result.output + assert "invalid" in result.output.lower() or "traversal" in result.output.lower() diff --git a/tests/workflows/test_resolver_integration.py b/tests/workflows/test_resolver_integration.py new file mode 100644 index 0000000000..cb14ec340f --- /dev/null +++ b/tests/workflows/test_resolver_integration.py @@ -0,0 +1,485 @@ +"""Integration tests for the WorkflowResolver.""" + +from __future__ import annotations + +from pathlib import Path + +import pytest +import yaml + +from specify_cli.workflows.engine import WorkflowDefinition +from specify_cli.workflows.overlays import WorkflowResolver +from specify_cli.workflows.overlays.merge import ComposedStep + + +def _write_workflow(project_root: Path, workflow_id: str, data: dict) -> Path: + wf_dir = project_root / ".specify" / "workflows" / workflow_id + wf_dir.mkdir(parents=True, exist_ok=True) + wf_path = wf_dir / "workflow.yml" + wf_path.write_text(yaml.safe_dump(data), encoding="utf-8") + return wf_path + + +def _write_overlay(project_root: Path, workflow_id: str, overlay_id: str, data: dict) -> Path: + ov_dir = project_root / ".specify" / "workflows" / "overlays" / workflow_id + ov_dir.mkdir(parents=True, exist_ok=True) + ov_path = ov_dir / f"{overlay_id}.yml" + ov_path.write_text(yaml.safe_dump(data), encoding="utf-8") + return ov_path + + +class TestWorkflowResolver: + """End-to-end resolution of base workflows plus overlays.""" + + def test_resolve_without_overlays(self, project_dir): + data = { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "speckit.specify"}], + } + _write_workflow(project_dir, "wf", data) + + resolver = WorkflowResolver(project_dir) + definition = resolver.resolve("wf") + assert isinstance(definition, WorkflowDefinition) + assert definition.id == "wf" + assert [s["id"] for s in definition.steps] == ["a"] + + def test_resolve_with_project_overlay_insert(self, project_dir): + data = { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [ + {"id": "a", "type": "command", "command": "speckit.specify"}, + {"id": "b", "type": "command", "command": "speckit.specify"}, + ], + } + _write_workflow(project_dir, "wf", data) + _write_overlay( + project_dir, + "wf", + "ov1", + { + "id": "ov1", + "extends": "wf", + "priority": 10, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "new", "type": "command", "command": "speckit.plan"}, + } + ], + }, + ) + + resolver = WorkflowResolver(project_dir) + definition = resolver.resolve("wf") + assert [s["id"] for s in definition.steps] == ["a", "new", "b"] + + def test_resolve_with_installed_overlay(self, project_dir): + data = { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "speckit.specify"}], + } + _write_workflow(project_dir, "wf", data) + installed_dir = project_dir / ".specify" / "workflows" / "wf" / "overlays" + installed_dir.mkdir(parents=True, exist_ok=True) + installed_dir.joinpath("shipped.yml").write_text( + yaml.safe_dump( + { + "id": "shipped", + "extends": "wf", + "priority": 5, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "shipped-new", "type": "command", "command": "echo"}, + } + ], + } + ), + encoding="utf-8", + ) + + resolver = WorkflowResolver(project_dir) + definition = resolver.resolve("wf") + assert [s["id"] for s in definition.steps] == ["a", "shipped-new"] + + def test_resolve_higher_priority_wins(self, project_dir): + data = { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "speckit.specify"}], + } + _write_workflow(project_dir, "wf", data) + _write_overlay( + project_dir, + "wf", + "low", + { + "id": "low", + "extends": "wf", + "priority": 5, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "low-step", "type": "command", "command": "echo"}, + } + ], + }, + ) + _write_overlay( + project_dir, + "wf", + "high", + { + "id": "high", + "extends": "wf", + "priority": 10, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "high-step", "type": "command", "command": "echo"}, + } + ], + }, + ) + + resolver = WorkflowResolver(project_dir) + definition = resolver.resolve("wf") + # Higher priority is applied later; both insert_after 'a', so high-step + # ends up closer to the anchor and wins the conflict. + assert [s["id"] for s in definition.steps] == ["a", "high-step", "low-step"] + + def test_resolve_project_wins_tie_with_installed(self, project_dir): + data = { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "speckit.specify"}], + } + _write_workflow(project_dir, "wf", data) + installed_dir = project_dir / ".specify" / "workflows" / "wf" / "overlays" + installed_dir.mkdir(parents=True, exist_ok=True) + installed_dir.joinpath("shipped.yml").write_text( + yaml.safe_dump( + { + "id": "shipped", + "extends": "wf", + "priority": 10, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "installed-step", "type": "command", "command": "echo"}, + } + ], + } + ), + encoding="utf-8", + ) + _write_overlay( + project_dir, + "wf", + "project", + { + "id": "project", + "extends": "wf", + "priority": 10, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "project-step", "type": "command", "command": "echo"}, + } + ], + }, + ) + + resolver = WorkflowResolver(project_dir) + definition = resolver.resolve("wf") + # Same priority: installed applied first, then project wins tie by + # ending up closer to the anchor. + assert [s["id"] for s in definition.steps] == ["a", "project-step", "installed-step"] + + def test_resolve_with_layers_returns_attribution(self, project_dir): + data = { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "speckit.specify"}], + } + _write_workflow(project_dir, "wf", data) + _write_overlay( + project_dir, + "wf", + "ov1", + { + "id": "ov1", + "extends": "wf", + "priority": 10, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "new", "type": "command", "command": "echo"}, + } + ], + }, + ) + + resolver = WorkflowResolver(project_dir) + definition, layers, attribution = resolver.resolve_with_layers("wf") + assert [s["id"] for s in definition.steps] == ["a", "new"] + assert any(layer.tier == "base" for layer in layers) + assert attribution == [ComposedStep("a", "base"), ComposedStep("new", "project:ov1")] + + def test_resolve_attribution_distinguishes_project_and_installed_same_id(self, project_dir): + data = { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "speckit.specify"}], + } + _write_workflow(project_dir, "wf", data) + installed_dir = project_dir / ".specify" / "workflows" / "wf" / "overlays" + installed_dir.mkdir(parents=True, exist_ok=True) + installed_dir.joinpath("same.yml").write_text( + yaml.safe_dump( + { + "id": "same", + "extends": "wf", + "priority": 10, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "installed-new", "type": "command", "command": "echo"}, + } + ], + } + ), + encoding="utf-8", + ) + _write_overlay( + project_dir, + "wf", + "same", + { + "id": "same", + "extends": "wf", + "priority": 10, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "project-new", "type": "command", "command": "echo"}, + } + ], + }, + ) + + resolver = WorkflowResolver(project_dir) + definition, _layers, attribution = resolver.resolve_with_layers("wf") + assert [s["id"] for s in definition.steps] == ["a", "project-new", "installed-new"] + sources = {c.step_id: c.source for c in attribution} + assert sources["project-new"] == "project:same" + assert sources["installed-new"] == "installed:same" + + def test_resolve_attribution_for_nested_base_steps(self, project_dir): + data = { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [ + { + "id": "if-1", + "type": "if", + "condition": "true", + "then": [{"id": "then-a", "type": "command", "command": "echo"}], + "else": [{"id": "else-b", "type": "command", "command": "echo"}], + } + ], + } + _write_workflow(project_dir, "wf", data) + + resolver = WorkflowResolver(project_dir) + definition, _layers, attribution = resolver.resolve_with_layers("wf") + assert [s["id"] for s in definition.steps] == ["if-1"] + sources = {c.step_id: c.source for c in attribution} + assert sources["if-1"] == "base" + assert sources["then-a"] == "base" + assert sources["else-b"] == "base" + + def test_resolve_invalid_project_overlay_fails(self, project_dir): + data = { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "speckit.specify"}], + } + _write_workflow(project_dir, "wf", data) + _write_overlay( + project_dir, + "wf", + "broken", + { + "id": "broken", + "extends": "wf", + "priority": 10, + "edits": "not-a-list", + }, + ) + + resolver = WorkflowResolver(project_dir) + with pytest.raises(ValueError): + resolver.resolve("wf") + + def test_resolve_invalid_installed_overlay_fails(self, project_dir): + data = { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "speckit.specify"}], + } + _write_workflow(project_dir, "wf", data) + installed_dir = project_dir / ".specify" / "workflows" / "wf" / "overlays" + installed_dir.mkdir(parents=True, exist_ok=True) + installed_dir.joinpath("broken.yml").write_text( + yaml.safe_dump( + { + "id": "broken", + "extends": "wf", + "priority": 10, + "edits": "not-a-list", + } + ), + encoding="utf-8", + ) + + resolver = WorkflowResolver(project_dir) + with pytest.raises(ValueError): + resolver.resolve("wf") + + def test_resolve_disabled_overlay_is_skipped(self, project_dir): + data = { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "speckit.specify"}], + } + _write_workflow(project_dir, "wf", data) + _write_overlay( + project_dir, + "wf", + "disabled", + { + "id": "disabled", + "extends": "wf", + "priority": 10, + "enabled": False, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "new", "type": "command", "command": "echo"}, + } + ], + }, + ) + + resolver = WorkflowResolver(project_dir) + definition = resolver.resolve("wf") + assert [s["id"] for s in definition.steps] == ["a"] + + def test_resolve_invalid_anchor_raises(self, project_dir): + data = { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "speckit.specify"}], + } + _write_workflow(project_dir, "wf", data) + _write_overlay( + project_dir, + "wf", + "ov1", + { + "id": "ov1", + "extends": "wf", + "priority": 10, + "edits": [ + { + "operation": "insert_after", + "anchor": "missing", + "step": {"id": "new", "type": "command", "command": "echo"}, + } + ], + }, + ) + + resolver = WorkflowResolver(project_dir) + with pytest.raises(ValueError, match="anchor 'missing' does not match any base step id"): + resolver.resolve("wf") + + def test_resolve_missing_workflow(self, project_dir): + resolver = WorkflowResolver(project_dir) + with pytest.raises(FileNotFoundError, match="Workflow not found"): + resolver.resolve("missing") + + def test_resolve_validates_composed_result(self, project_dir): + data = { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "speckit.specify"}], + } + _write_workflow(project_dir, "wf", data) + _write_overlay( + project_dir, + "wf", + "ov1", + { + "id": "ov1", + "extends": "wf", + "priority": 10, + "edits": [ + { + "operation": "replace", + "anchor": "a", + "step": {"id": "a", "type": "invalid-type", "command": "echo"}, + } + ], + }, + ) + + resolver = WorkflowResolver(project_dir) + with pytest.raises(ValueError, match="Composed workflow is invalid"): + resolver.resolve("wf") + + def test_engine_load_workflow_uses_resolver(self, project_dir): + from specify_cli.workflows.engine import WorkflowEngine + + data = { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "speckit.specify"}], + } + _write_workflow(project_dir, "wf", data) + _write_overlay( + project_dir, + "wf", + "ov1", + { + "id": "ov1", + "extends": "wf", + "priority": 10, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "new", "type": "command", "command": "echo"}, + } + ], + }, + ) + + engine = WorkflowEngine(project_dir) + definition = engine.load_workflow("wf") + assert [s["id"] for s in definition.steps] == ["a", "new"] From 01e0455f254e2f605424db49a307fbfa5cd95856 Mon Sep 17 00:00:00 2001 From: Markus Date: Thu, 16 Jul 2026 14:47:38 +0200 Subject: [PATCH 02/29] fix(workflows): reject symlinked overlay directories in layer sources Address PR #3557 review comments r3594064534 and r3594064563: - ProjectOverlaySource.collect now rejects symlinked per-workflow overlay directories (.specify/workflows/overlays/) before iterating - InstalledOverlaySource.collect now rejects symlinked installed overlay directories (.specify/workflows//overlays) before iterating - workflow_overlay_list catches ValueError from resolver and exits with code 1 instead of crashing on unhandled exceptions - Added .specify/workflows/overlays to _reject_unsafe_workflow_storage chokepoint for defense-in-depth These guards prevent symlinked overlay directories from redirecting auto-loaded overlay YAML to attacker-controlled content outside the project, which could inject executable shell steps into trusted workflows. Refs: PR #3557 review comments r3594064534, r3594064563 Assisted-by: opencode-go/qwen3.7-max (autonomous) --- src/specify_cli/workflows/_commands.py | 7 +- .../workflows/overlays/_commands.py | 10 ++- .../workflows/overlays/layer_sources.py | 10 +++ tests/workflows/test_overlay_security.py | 84 +++++++++++++++++++ tests/workflows/test_resolver_integration.py | 79 +++++++++++++++++ 5 files changed, 186 insertions(+), 4 deletions(-) diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index 384f08c5c3..0a5a43fbcf 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -109,6 +109,10 @@ def _reject_unsafe_workflow_storage(project_root: Path) -> None: project_root / ".specify" / "workflows" / "runs", ".specify/workflows/runs", ) + _reject_unsafe_dir( + project_root / ".specify" / "workflows" / "overlays", + ".specify/workflows/overlays", + ) _WORKFLOW_ID_PATTERN = re.compile(r"^[a-z0-9](?:[a-z0-9-]*[a-z0-9])?$") @@ -1818,7 +1822,8 @@ def workflow_overlay_list_cmd( from .overlays._commands import workflow_overlay_list project_root = _require_specify_project() - workflow_overlay_list(project_root, workflow_id) + if workflow_overlay_list(project_root, workflow_id) is None: + raise typer.Exit(1) @workflow_app.command("resolve") diff --git a/src/specify_cli/workflows/overlays/_commands.py b/src/specify_cli/workflows/overlays/_commands.py index bccb7872be..3686d02e37 100644 --- a/src/specify_cli/workflows/overlays/_commands.py +++ b/src/specify_cli/workflows/overlays/_commands.py @@ -284,14 +284,18 @@ def workflow_overlay_remove( return True -def workflow_overlay_list(project_root: Path, workflow_id: str) -> list[dict[str, Any]]: +def workflow_overlay_list(project_root: Path, workflow_id: str) -> list[dict[str, Any]] | None: """List all overlays for a workflow and print a summary table. - Returns the raw list data for machine-readable callers. + Returns the raw list data for machine-readable callers, or None on error. """ _validate_workflow_id_or_exit(workflow_id) resolver = WorkflowResolver(project_root) - layers = resolver.collect_all_layers(workflow_id) + try: + layers = resolver.collect_all_layers(workflow_id) + except ValueError as exc: + err_console.print(f"[red]Error:[/red] {exc}") + return None overlays = [layer for layer in layers if layer.tier != "base"] if not overlays: diff --git a/src/specify_cli/workflows/overlays/layer_sources.py b/src/specify_cli/workflows/overlays/layer_sources.py index 8f4082c612..a5d76f14a7 100644 --- a/src/specify_cli/workflows/overlays/layer_sources.py +++ b/src/specify_cli/workflows/overlays/layer_sources.py @@ -42,6 +42,11 @@ def __init__(self, project_root: Path) -> None: def collect(self, workflow_id: str) -> list[Layer]: """Collect all project-local overlays for the given workflow id.""" workflow_overlay_dir = self.overlays_dir / workflow_id + if workflow_overlay_dir.is_symlink(): + raise OverlayLoadError( + workflow_overlay_dir, + ["Symlinked overlay directories are not allowed"], + ) if not workflow_overlay_dir.is_dir(): return [] layers: list[Layer] = [] @@ -82,6 +87,11 @@ def __init__(self, project_root: Path) -> None: def collect(self, workflow_id: str) -> list[Layer]: """Collect all installed overlays shipped with the given workflow.""" installed_overlay_dir = self.workflows_dir / workflow_id / "overlays" + if installed_overlay_dir.is_symlink(): + raise OverlayLoadError( + installed_overlay_dir, + ["Symlinked overlay directories are not allowed"], + ) if not installed_overlay_dir.is_dir(): return [] layers: list[Layer] = [] diff --git a/tests/workflows/test_overlay_security.py b/tests/workflows/test_overlay_security.py index 3871d89217..e41b7ff8d0 100644 --- a/tests/workflows/test_overlay_security.py +++ b/tests/workflows/test_overlay_security.py @@ -202,3 +202,87 @@ def test_overlay_enable_rejects_traversal(self, project_dir, monkeypatch): result = runner.invoke(app, ["workflow", "overlay", "enable", "wf", "../other"]) assert result.exit_code != 0, result.output assert "invalid" in result.output.lower() or "traversal" in result.output.lower() + + def test_overlay_rejects_symlinked_overlays_dir(self, project_dir, monkeypatch, tmp_path): + """Overlay commands must reject a symlinked .specify/workflows/overlays directory.""" + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + + # Create a symlinked overlays directory pointing outside the project + outside_dir = tmp_path / "outside" + outside_dir.mkdir() + overlays_dir = project_dir / ".specify" / "workflows" / "overlays" + overlays_dir.symlink_to(outside_dir) + + result = runner.invoke(app, ["workflow", "overlay", "list", "wf"]) + assert result.exit_code != 0, result.output + assert "symlink" in result.output.lower() + + def test_overlay_list_rejects_symlinked_per_workflow_dir(self, project_dir, monkeypatch, tmp_path): + """Overlay list must reject a symlinked per-workflow overlay directory.""" + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + + # Create a real overlay directory outside the project. + outside_dir = tmp_path / "outside_wf" + outside_dir.mkdir() + outside_dir.joinpath("evil.yml").write_text( + yaml.safe_dump( + { + "id": "evil", + "extends": "wf", + "priority": 100, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "evil-step", "type": "command", "command": "echo"}, + } + ], + } + ), + encoding="utf-8", + ) + + # Symlink the per-workflow overlay directory to the outside location. + overlays_root = project_dir / ".specify" / "workflows" / "overlays" + overlays_root.mkdir(parents=True, exist_ok=True) + symlink_dir = overlays_root / "wf" + symlink_dir.symlink_to(outside_dir) + + result = runner.invoke(app, ["workflow", "overlay", "list", "wf"]) + assert result.exit_code != 0, result.output + assert "symlink" in result.output.lower() + + def test_overlay_list_rejects_symlinked_installed_overlays_dir(self, project_dir, monkeypatch, tmp_path): + """Overlay list must reject a symlinked installed overlays directory.""" + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + + # Create a real overlays directory outside the project. + outside_dir = tmp_path / "outside_installed" + outside_dir.mkdir() + outside_dir.joinpath("evil.yml").write_text( + yaml.safe_dump( + { + "id": "evil", + "extends": "wf", + "priority": 100, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "evil-step", "type": "command", "command": "echo"}, + } + ], + } + ), + encoding="utf-8", + ) + + # Create a workflow directory and symlink its overlays/ to the outside. + wf_dir = project_dir / ".specify" / "workflows" / "wf" + wf_dir.mkdir(parents=True, exist_ok=True) + symlink_dir = wf_dir / "overlays" + symlink_dir.symlink_to(outside_dir) + + result = runner.invoke(app, ["workflow", "overlay", "list", "wf"]) + assert result.exit_code != 0, result.output + assert "symlink" in result.output.lower() diff --git a/tests/workflows/test_resolver_integration.py b/tests/workflows/test_resolver_integration.py index cb14ec340f..b7ee052ef6 100644 --- a/tests/workflows/test_resolver_integration.py +++ b/tests/workflows/test_resolver_integration.py @@ -453,6 +453,85 @@ def test_resolve_validates_composed_result(self, project_dir): with pytest.raises(ValueError, match="Composed workflow is invalid"): resolver.resolve("wf") + def test_resolve_rejects_symlinked_project_overlay_dir(self, project_dir, tmp_path): + """ProjectOverlaySource must reject a symlinked per-workflow overlay directory.""" + data = { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "speckit.specify"}], + } + _write_workflow(project_dir, "wf", data) + + # Create a real overlay directory outside the project with a malicious overlay. + outside_dir = tmp_path / "outside_overlays" / "wf" + outside_dir.mkdir(parents=True, exist_ok=True) + outside_dir.joinpath("evil.yml").write_text( + yaml.safe_dump( + { + "id": "evil", + "extends": "wf", + "priority": 100, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "evil-step", "type": "command", "command": "rm -rf /"}, + } + ], + } + ), + encoding="utf-8", + ) + + # Symlink the per-workflow overlay directory to the outside location. + overlays_root = project_dir / ".specify" / "workflows" / "overlays" + overlays_root.mkdir(parents=True, exist_ok=True) + symlink_dir = overlays_root / "wf" + symlink_dir.symlink_to(outside_dir) + + resolver = WorkflowResolver(project_dir) + with pytest.raises(ValueError, match="Symlinked overlay directories are not allowed"): + resolver.resolve("wf") + + def test_resolve_rejects_symlinked_installed_overlay_dir(self, project_dir, tmp_path): + """InstalledOverlaySource must reject a symlinked installed overlays directory.""" + data = { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "speckit.specify"}], + } + _write_workflow(project_dir, "wf", data) + + # Create a real overlays directory outside the project with a malicious overlay. + outside_dir = tmp_path / "outside_installed" + outside_dir.mkdir(parents=True, exist_ok=True) + outside_dir.joinpath("evil.yml").write_text( + yaml.safe_dump( + { + "id": "evil", + "extends": "wf", + "priority": 100, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "evil-step", "type": "command", "command": "rm -rf /"}, + } + ], + } + ), + encoding="utf-8", + ) + + # Symlink the installed overlays directory to the outside location. + wf_dir = project_dir / ".specify" / "workflows" / "wf" + symlink_dir = wf_dir / "overlays" + symlink_dir.symlink_to(outside_dir) + + resolver = WorkflowResolver(project_dir) + with pytest.raises(ValueError, match="Symlinked overlay directories are not allowed"): + resolver.resolve("wf") + def test_engine_load_workflow_uses_resolver(self, project_dir): from specify_cli.workflows.engine import WorkflowEngine From c70a5d61cf32e161244d26766ceaa487faf9747d Mon Sep 17 00:00:00 2001 From: Markus Date: Thu, 16 Jul 2026 15:04:15 +0200 Subject: [PATCH 03/29] fix(workflows): address Copilot review findings in merge engine - Apply inserts before winning replace to prevent anchor-not-found errors when replace changes step ID (r3594064604) - Track attribution recursively for nested steps in composite inserts/replaces so workflow resolve attributes all child steps correctly (r3594064638) - Add regression tests for both fixes Refs: PR #3557 review discussion Assisted-by: GitHub Copilot (model: qwen3.7-plus, autonomous) --- src/specify_cli/workflows/overlays/merge.py | 82 +++++++-- tests/workflows/test_overlay_merge.py | 178 +++++++++++++++++++ tests/workflows/test_resolver_integration.py | 40 +++++ 3 files changed, 288 insertions(+), 12 deletions(-) diff --git a/src/specify_cli/workflows/overlays/merge.py b/src/specify_cli/workflows/overlays/merge.py index 73a9c9f3e6..3b11cd1e0c 100644 --- a/src/specify_cli/workflows/overlays/merge.py +++ b/src/specify_cli/workflows/overlays/merge.py @@ -102,6 +102,61 @@ def _init_sources_recursively( _init_sources_recursively(case_steps, sources) +def _record_sources_recursively( + step: dict[str, Any], + source: str, + sources: dict[str, str], +) -> None: + """Record *source* for a step and all its nested child steps. + + Traverses ``then``, ``else``, ``steps``, ``default``, and ``cases.*`` + so that ``workflow resolve`` attributes every step inside a composite + insert or replacement to the correct overlay layer. + """ + step_id = step.get("id") + if isinstance(step_id, str): + sources[step_id] = source + for key in _NESTED_LIST_KEYS: + nested = step.get(key) + if isinstance(nested, list): + for child in nested: + if isinstance(child, dict): + _record_sources_recursively(child, source, sources) + cases = step.get("cases") + if isinstance(cases, dict): + for case_steps in cases.values(): + if isinstance(case_steps, list): + for child in case_steps: + if isinstance(child, dict): + _record_sources_recursively(child, source, sources) + + +def _remove_sources_recursively( + step: dict[str, Any], + sources: dict[str, str], +) -> None: + """Remove source entries for a step and all its nested child steps. + + Traverses the same nesting keys as ``_record_sources_recursively``. + """ + step_id = step.get("id") + if isinstance(step_id, str): + sources.pop(step_id, None) + for key in _NESTED_LIST_KEYS: + nested = step.get(key) + if isinstance(nested, list): + for child in nested: + if isinstance(child, dict): + _remove_sources_recursively(child, sources) + cases = step.get("cases") + if isinstance(cases, dict): + for case_steps in cases.values(): + if isinstance(case_steps, list): + for child in case_steps: + if isinstance(child, dict): + _remove_sources_recursively(child, sources) + + def apply_edit( steps: list[dict[str, Any]], edit: OverlayEdit, @@ -207,30 +262,33 @@ def merge_steps( if location is not None: parent_list, index = location removed_step = parent_list[index] - removed_id = removed_step.get("id") if isinstance(removed_step, dict) else None del parent_list[index] - if isinstance(removed_id, str): - sources.pop(removed_id, None) + if isinstance(removed_step, dict): + _remove_sources_recursively(removed_step, sources) continue # For replace/insert_*, the anchor survives. Only the highest-priority # replace is applied; lower-priority replaces on the same anchor are # skipped. Inserts are applied in merge order. - if winning_edit.operation == "replace": - winning_layer, _ = edits[-1] - steps, composed, replaced_id = apply_edit(steps, winning_edit, winning_layer.source) - if isinstance(replaced_id, str): - sources.pop(replaced_id, None) - if composed is not None: - sources[composed.step_id] = composed.source - + # + # Inserts must be applied *before* the winning replace: if the replace + # changes the step ID, ``find_step`` can no longer locate the original + # anchor and the inserts would raise. for layer, edit in edits: if edit.operation in ("insert_after", "insert_before"): steps, composed, replaced_id = apply_edit(steps, edit, layer.source) if replaced_id is not None: sources.pop(replaced_id, None) if composed is not None: - sources[composed.step_id] = composed.source + _record_sources_recursively(edit.step, composed.source, sources) + + if winning_edit.operation == "replace": + winning_layer, _ = edits[-1] + steps, composed, replaced_id = apply_edit(steps, winning_edit, winning_layer.source) + if isinstance(replaced_id, str): + sources.pop(replaced_id, None) + if composed is not None: + _record_sources_recursively(winning_edit.step, composed.source, sources) attribution = _build_attribution(steps, sources) return steps, attribution diff --git a/tests/workflows/test_overlay_merge.py b/tests/workflows/test_overlay_merge.py index 01be81bf48..9eb0db3b12 100644 --- a/tests/workflows/test_overlay_merge.py +++ b/tests/workflows/test_overlay_merge.py @@ -364,6 +364,60 @@ def test_merge_steps_project_wins_tie_over_installed_same_anchor(self): for composed in attribution ) + def test_merge_steps_insert_after_then_replace_same_anchor_id_change(self): + """Inserts must be applied before the winning replace so the anchor still exists. + + Regression: when a replace changes the step ID, applying it before inserts + causes ``find_step`` to fail on the now-gone original anchor. + """ + base = [_step("build")] + low = Overlay( + id="low", + extends="wf", + priority=5, + edits=[OverlayEdit("insert_after", "build", _step("test"))], + ) + high = Overlay( + id="high", + extends="wf", + priority=10, + edits=[OverlayEdit("replace", "build", _step("compile"))], + ) + steps, attribution = merge_steps( + base, [_layer(low, "project:low"), _layer(high, "project:high")] + ) + # The insert should land after the original anchor position, then the + # anchor is replaced. Final order: ["compile", "test"]. + assert [s["id"] for s in steps] == ["compile", "test"] + assert attribution == [ + ComposedStep("compile", "project:high"), + ComposedStep("test", "project:low"), + ] + + def test_merge_steps_insert_before_then_replace_same_anchor_id_change(self): + """Same as above but with insert_before — anchor must still be findable.""" + base = [_step("build")] + low = Overlay( + id="low", + extends="wf", + priority=5, + edits=[OverlayEdit("insert_before", "build", _step("lint"))], + ) + high = Overlay( + id="high", + extends="wf", + priority=10, + edits=[OverlayEdit("replace", "build", _step("compile"))], + ) + steps, attribution = merge_steps( + base, [_layer(low, "project:low"), _layer(high, "project:high")] + ) + assert [s["id"] for s in steps] == ["lint", "compile"] + assert attribution == [ + ComposedStep("lint", "project:low"), + ComposedStep("compile", "project:high"), + ] + def test_merge_steps_unknown_anchor_still_raises(self): base = [_step("a")] overlay = Overlay( @@ -375,6 +429,130 @@ def test_merge_steps_unknown_anchor_still_raises(self): with pytest.raises(ValueError, match="Anchor 'missing' not found"): merge_steps(base, [_layer(overlay, "project:ov")]) + # ── composite step attribution ─────────────────────────────────────── + + def test_merge_insert_composite_if_attribution(self): + """Nested then/else children of an inserted 'if' step get the overlay source.""" + base = [_step("a")] + composite = { + "id": "if-1", + "type": "if", + "condition": "true", + "then": [_step("then-a")], + "else": [_step("else-b")], + } + overlay = Overlay( + id="ov", extends="wf", priority=10, + edits=[OverlayEdit("insert_after", "a", composite)], + ) + _steps, attribution = merge_steps( + base, [_layer(overlay, "project:ov")] + ) + assert attribution == [ + ComposedStep("a", "base"), + ComposedStep("if-1", "project:ov"), + ComposedStep("then-a", "project:ov"), + ComposedStep("else-b", "project:ov"), + ] + + def test_merge_insert_composite_switch_attribution(self): + """Nested cases/default children of an inserted 'switch' step get the overlay source.""" + base = [_step("a")] + composite = { + "id": "switch-1", + "type": "switch", + "expression": "{{inputs.x}}", + "cases": {"one": [_step("case-one")], "two": [_step("case-two")]}, + "default": [_step("default-z")], + } + overlay = Overlay( + id="ov", extends="wf", priority=10, + edits=[OverlayEdit("insert_before", "a", composite)], + ) + _steps, attribution = merge_steps( + base, [_layer(overlay, "project:ov")] + ) + assert attribution == [ + ComposedStep("switch-1", "project:ov"), + ComposedStep("default-z", "project:ov"), + ComposedStep("case-one", "project:ov"), + ComposedStep("case-two", "project:ov"), + ComposedStep("a", "base"), + ] + + def test_merge_replace_flat_with_composite_attribution(self): + """Replacing a flat step with a composite step attributes all nested children.""" + base = [_step("a")] + composite = { + "id": "if-1", + "type": "if", + "condition": "true", + "then": [_step("inner-x"), _step("inner-y")], + } + overlay = Overlay( + id="ov", extends="wf", priority=10, + edits=[OverlayEdit("replace", "a", composite)], + ) + _steps, attribution = merge_steps( + base, [_layer(overlay, "project:ov")] + ) + assert attribution == [ + ComposedStep("if-1", "project:ov"), + ComposedStep("inner-x", "project:ov"), + ComposedStep("inner-y", "project:ov"), + ] + + def test_merge_remove_composite_step_cleans_nested_sources(self): + """Removing a composite step also cleans its nested children from sources.""" + base = [ + { + "id": "if-1", + "type": "if", + "condition": "true", + "then": [_step("then-a")], + "else": [_step("else-b")], + }, + _step("a"), + ] + overlay = Overlay( + id="ov", extends="wf", priority=10, + edits=[OverlayEdit("remove", "if-1")], + ) + steps, attribution = merge_steps( + base, [_layer(overlay, "project:ov")] + ) + assert [s["id"] for s in steps] == ["a"] + assert attribution == [ComposedStep("a", "base")] + + def test_merge_insert_deeply_nested_composite_attribution(self): + """Deep nesting (if inside while) gets the overlay source at every level.""" + base = [_step("a")] + inner_if = { + "id": "inner-if", + "type": "if", + "condition": "true", + "then": [_step("deep-x")], + } + composite = { + "id": "while-1", + "type": "while", + "condition": "true", + "steps": [inner_if], + } + overlay = Overlay( + id="ov", extends="wf", priority=10, + edits=[OverlayEdit("insert_after", "a", composite)], + ) + _steps, attribution = merge_steps( + base, [_layer(overlay, "project:ov")] + ) + assert attribution == [ + ComposedStep("a", "base"), + ComposedStep("while-1", "project:ov"), + ComposedStep("inner-if", "project:ov"), + ComposedStep("deep-x", "project:ov"), + ] + class TestValidateEdits: """Edit validation against known base step IDs.""" diff --git a/tests/workflows/test_resolver_integration.py b/tests/workflows/test_resolver_integration.py index b7ee052ef6..91f4b14e65 100644 --- a/tests/workflows/test_resolver_integration.py +++ b/tests/workflows/test_resolver_integration.py @@ -532,6 +532,46 @@ def test_resolve_rejects_symlinked_installed_overlay_dir(self, project_dir, tmp_ with pytest.raises(ValueError, match="Symlinked overlay directories are not allowed"): resolver.resolve("wf") + def test_resolve_attribution_for_inserted_composite_step(self, project_dir): + """Inserted composite steps must attribute nested children to the overlay source.""" + data = { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "speckit.specify"}], + } + _write_workflow(project_dir, "wf", data) + _write_overlay( + project_dir, + "wf", + "ov1", + { + "id": "ov1", + "extends": "wf", + "priority": 10, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": { + "id": "if-1", + "type": "if", + "condition": "true", + "then": [{"id": "then-x", "type": "command", "command": "echo"}], + "else": [{"id": "else-y", "type": "command", "command": "echo"}], + }, + } + ], + }, + ) + + resolver = WorkflowResolver(project_dir) + _definition, _layers, attribution = resolver.resolve_with_layers("wf") + sources = {c.step_id: c.source for c in attribution} + assert sources["a"] == "base" + assert sources["if-1"] == "project:ov1" + assert sources["then-x"] == "project:ov1" + assert sources["else-y"] == "project:ov1" + def test_engine_load_workflow_uses_resolver(self, project_dir): from specify_cli.workflows.engine import WorkflowEngine From cc281856d696914cae3efcd5e5cf8410e77c57f3 Mon Sep 17 00:00:00 2001 From: Markus Date: Thu, 16 Jul 2026 16:24:00 +0200 Subject: [PATCH 04/29] refactor(workflows): simplify overlay architecture to 2-tier Remove installed overlays tier to enforce clean separation of concerns: - workflow add installs workflows only (no overlay copying) - workflow overlay add installs overlays only (project-local) Changes: - Remove InstalledOverlaySource class and all references - Remove overlay-copying logic from _validate_and_install_local() - Update WorkflowResolver to 2-tier: project overlays + base workflow - Fix --priority override timing: apply before validation, not after - Remove tests for installed overlays (no longer applicable) Rationale: If upstream controls both base workflow and shipped overlays, and both get overwritten on bundle update, there's no reason to ship overlays separately. Overlays only make sense when someone other than the base author adds them. Resolves all three review findings from PR #3557: - r3594064677: workflow add no longer copies overlays from all call sites - r3594064705: --priority override now applied before validation - r3594064726: no stale installed overlays (tier removed entirely) Assisted-by: Claude (model: claude-opus-4-7, autonomous) --- src/specify_cli/workflows/_commands.py | 10 - .../workflows/overlays/__init__.py | 14 +- .../workflows/overlays/_commands.py | 17 +- .../workflows/overlays/layer_sources.py | 45 ---- tests/workflows/test_overlay_commands.py | 46 +++- tests/workflows/test_overlay_merge.py | 25 +-- tests/workflows/test_overlay_security.py | 35 ---- tests/workflows/test_resolver_integration.py | 196 ------------------ 8 files changed, 67 insertions(+), 321 deletions(-) diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index 0a5a43fbcf..994952aec6 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -637,16 +637,6 @@ def _validate_and_install_local(yaml_path: Path, source_label: str) -> None: import shutil shutil.copy2(yaml_path, dest_dir / "workflow.yml") - # If the source is a directory that also contains an overlays/ subdirectory, - # copy it alongside the workflow.yml so installed overlays are preserved. - source_dir = yaml_path.parent - source_overlays = source_dir / "overlays" - if source_overlays.is_dir() and not source_overlays.is_symlink(): - dest_overlays = dest_dir / "overlays" - if dest_overlays.exists(): - shutil.rmtree(dest_overlays) - shutil.copytree(source_overlays, dest_overlays) - registry.add(definition.id, { "name": definition.name, "version": definition.version, diff --git a/src/specify_cli/workflows/overlays/__init__.py b/src/specify_cli/workflows/overlays/__init__.py index 67929b0a6f..945ba40d02 100644 --- a/src/specify_cli/workflows/overlays/__init__.py +++ b/src/specify_cli/workflows/overlays/__init__.py @@ -8,7 +8,6 @@ from .composer import StepListComposer from .layer_sources import ( BaseWorkflowSource, - InstalledOverlaySource, Layer, ProjectOverlaySource, ) @@ -18,9 +17,8 @@ class WorkflowResolver: """Resolves a workflow ID to its composed ``WorkflowDefinition``. - Collects layers from three tiers: + Collects layers from two tiers: - project-local overlays (``.specify/workflows/overlays//*.yml``) - - installed overlays shipped with a workflow (``.specify/workflows//overlays/*.yml``) - the base workflow itself (``.specify/workflows//workflow.yml``) Resolution is higher-wins: overlays with higher priority are applied @@ -31,7 +29,6 @@ def __init__(self, project_root: Path) -> None: self.project_root = project_root self._sources = [ ProjectOverlaySource(project_root), - InstalledOverlaySource(project_root), BaseWorkflowSource(project_root), ] self._composer = StepListComposer() @@ -39,8 +36,7 @@ def __init__(self, project_root: Path) -> None: def collect_all_layers(self, workflow_id: str) -> list[Layer]: """Collect all layers sorted by resolver precedence. - Higher priority wins. Ties are broken so project overlays rank above - installed overlays (they are listed first in higher-wins order). + Higher priority wins. Ties are broken alphabetically by source. """ all_layers: list[Layer] = [] for source in self._sources: @@ -48,11 +44,7 @@ def collect_all_layers(self, workflow_id: str) -> list[Layer]: return sorted( all_layers, - key=lambda layer: ( - -layer.priority, - 0 if layer.tier == "project-overlay" else 1, - layer.source, - ), + key=lambda layer: (-layer.priority, layer.source), ) def resolve(self, workflow_id: str) -> WorkflowDefinition: diff --git a/src/specify_cli/workflows/overlays/_commands.py b/src/specify_cli/workflows/overlays/_commands.py index 3686d02e37..04a5931f3f 100644 --- a/src/specify_cli/workflows/overlays/_commands.py +++ b/src/specify_cli/workflows/overlays/_commands.py @@ -147,6 +147,12 @@ def workflow_overlay_add( err_console.print(f"[red]Error:[/red] {err}") return None + # Apply --priority override before validation so a valid CLI priority + # can fix a missing or invalid priority in the file. + if priority is not None: + _validate_priority(priority) + data["priority"] = priority + overlay, validation_errors = validate_overlay_yaml(data) if overlay is None: err_console.print("[red]Error:[/red] Overlay validation failed:") @@ -154,17 +160,6 @@ def workflow_overlay_add( err_console.print(f" \u2022 {err}") return None - if priority is not None: - _validate_priority(priority) - data["priority"] = priority - # Re-validate after mutation. - overlay, validation_errors = validate_overlay_yaml(data) - if overlay is None: - err_console.print("[red]Error:[/red] Overlay validation failed:") - for err in validation_errors: - err_console.print(f" \u2022 {err}") - return None - target_dir = _project_overlay_dir(project_root, overlay.extends) target_dir.mkdir(parents=True, exist_ok=True) target_path = _ensure_contained_path( diff --git a/src/specify_cli/workflows/overlays/layer_sources.py b/src/specify_cli/workflows/overlays/layer_sources.py index a5d76f14a7..98a69c8234 100644 --- a/src/specify_cli/workflows/overlays/layer_sources.py +++ b/src/specify_cli/workflows/overlays/layer_sources.py @@ -75,51 +75,6 @@ def collect(self, workflow_id: str) -> list[Layer]: return layers -class InstalledOverlaySource: - """Installed overlays: ``.specify/workflows//overlays/*.yml``.""" - - tier = "installed-overlay" - - def __init__(self, project_root: Path) -> None: - self.project_root = project_root - self.workflows_dir = project_root / ".specify" / "workflows" - - def collect(self, workflow_id: str) -> list[Layer]: - """Collect all installed overlays shipped with the given workflow.""" - installed_overlay_dir = self.workflows_dir / workflow_id / "overlays" - if installed_overlay_dir.is_symlink(): - raise OverlayLoadError( - installed_overlay_dir, - ["Symlinked overlay directories are not allowed"], - ) - if not installed_overlay_dir.is_dir(): - return [] - layers: list[Layer] = [] - for path in sorted(installed_overlay_dir.iterdir()): - if not path.is_file() or path.suffix not in (".yml", ".yaml"): - continue - if path.is_symlink(): - raise OverlayLoadError(path, ["Symlinked overlay files are not allowed"]) - data = yaml.safe_load(path.read_text(encoding="utf-8")) or {} - overlay, errors = validate_overlay_yaml(data) - if overlay is None or errors: - raise OverlayLoadError(path, errors) - if not overlay.enabled: - continue - if overlay.extends != workflow_id: - continue - layers.append( - Layer( - content=overlay, - source=f"installed:{overlay.id}", - tier=self.tier, - priority=overlay.priority, - path=path, - ) - ) - return layers - - class BaseWorkflowSource: """Base workflow layer: ``.specify/workflows//workflow.yml``.""" diff --git a/tests/workflows/test_overlay_commands.py b/tests/workflows/test_overlay_commands.py index 3e4aad9131..5e606f3a53 100644 --- a/tests/workflows/test_overlay_commands.py +++ b/tests/workflows/test_overlay_commands.py @@ -82,6 +82,48 @@ def test_overlay_add(self, project_dir, monkeypatch): data = yaml.safe_load(installed.read_text(encoding="utf-8")) assert data["priority"] == 5 + def test_overlay_add_with_priority_override_missing_in_file(self, project_dir, monkeypatch): + """--priority must fix a missing priority in the overlay file.""" + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + _write_workflow( + project_dir, + "wf", + { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "echo"}], + }, + ) + # Overlay file has NO priority field + overlay_file = project_dir / "overlay.yml" + overlay_file.write_text( + yaml.safe_dump( + { + "id": "ov1", + "extends": "wf", + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "new", "type": "command", "command": "echo"}, + } + ], + } + ), + encoding="utf-8", + ) + + result = runner.invoke( + app, ["workflow", "overlay", "add", str(overlay_file), "--priority", "5"] + ) + assert result.exit_code == 0, result.output + assert "Overlay 'ov1' added" in result.output + + installed = project_dir / ".specify" / "workflows" / "overlays" / "wf" / "ov1.yml" + assert installed.is_file() + data = yaml.safe_load(installed.read_text(encoding="utf-8")) + assert data["priority"] == 5 + def test_overlay_set_priority(self, project_dir, monkeypatch): monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) _write_workflow( @@ -308,7 +350,9 @@ def test_workflow_add_copies_overlays(self, project_dir, monkeypatch, tmp_path): result = runner.invoke(app, ["workflow", "add", str(source_dir)]) assert result.exit_code == 0, result.output + # Overlays in the source directory should NOT be copied — workflow add + # only installs the workflow.yml, not sibling overlays. installed_overlay = ( project_dir / ".specify" / "workflows" / "wf" / "overlays" / "ov1.yml" ) - assert installed_overlay.is_file() + assert not installed_overlay.exists() diff --git a/tests/workflows/test_overlay_merge.py b/tests/workflows/test_overlay_merge.py index 9eb0db3b12..b45bc73767 100644 --- a/tests/workflows/test_overlay_merge.py +++ b/tests/workflows/test_overlay_merge.py @@ -336,31 +336,32 @@ def test_merge_steps_higher_insert_wins_after_lower_remove_same_anchor(self): ComposedStep("high-after", "project:high"), ] - def test_merge_steps_project_wins_tie_over_installed_same_anchor(self): + def test_merge_steps_later_overlay_wins_tie_same_anchor(self): + """When two overlays have the same priority, the one applied later wins.""" base = [_step("a")] - installed = Overlay( - id="same", + first = Overlay( + id="first", extends="wf", priority=10, - edits=[OverlayEdit("replace", "a", _step("installed-replace"))], + edits=[OverlayEdit("replace", "a", _step("first-replace"))], ) - project = Overlay( - id="same", + second = Overlay( + id="second", extends="wf", priority=10, - edits=[OverlayEdit("replace", "a", _step("project-replace"))], + edits=[OverlayEdit("replace", "a", _step("second-replace"))], ) - # Merge order: installed first, then project (project wins tie). + # Merge order: first applied, then second wins tie. steps, attribution = merge_steps( base, [ - _layer(installed, "installed:same"), - _layer(project, "project:same"), + _layer(first, "overlay:first"), + _layer(second, "overlay:second"), ], ) - assert [s["id"] for s in steps] == ["project-replace"] + assert [s["id"] for s in steps] == ["second-replace"] assert any( - composed.step_id == "project-replace" and composed.source == "project:same" + composed.step_id == "second-replace" and composed.source == "overlay:second" for composed in attribution ) diff --git a/tests/workflows/test_overlay_security.py b/tests/workflows/test_overlay_security.py index e41b7ff8d0..2d392cd1bc 100644 --- a/tests/workflows/test_overlay_security.py +++ b/tests/workflows/test_overlay_security.py @@ -251,38 +251,3 @@ def test_overlay_list_rejects_symlinked_per_workflow_dir(self, project_dir, monk result = runner.invoke(app, ["workflow", "overlay", "list", "wf"]) assert result.exit_code != 0, result.output assert "symlink" in result.output.lower() - - def test_overlay_list_rejects_symlinked_installed_overlays_dir(self, project_dir, monkeypatch, tmp_path): - """Overlay list must reject a symlinked installed overlays directory.""" - monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) - - # Create a real overlays directory outside the project. - outside_dir = tmp_path / "outside_installed" - outside_dir.mkdir() - outside_dir.joinpath("evil.yml").write_text( - yaml.safe_dump( - { - "id": "evil", - "extends": "wf", - "priority": 100, - "edits": [ - { - "operation": "insert_after", - "anchor": "a", - "step": {"id": "evil-step", "type": "command", "command": "echo"}, - } - ], - } - ), - encoding="utf-8", - ) - - # Create a workflow directory and symlink its overlays/ to the outside. - wf_dir = project_dir / ".specify" / "workflows" / "wf" - wf_dir.mkdir(parents=True, exist_ok=True) - symlink_dir = wf_dir / "overlays" - symlink_dir.symlink_to(outside_dir) - - result = runner.invoke(app, ["workflow", "overlay", "list", "wf"]) - assert result.exit_code != 0, result.output - assert "symlink" in result.output.lower() diff --git a/tests/workflows/test_resolver_integration.py b/tests/workflows/test_resolver_integration.py index 91f4b14e65..b11fc6bed8 100644 --- a/tests/workflows/test_resolver_integration.py +++ b/tests/workflows/test_resolver_integration.py @@ -77,37 +77,6 @@ def test_resolve_with_project_overlay_insert(self, project_dir): definition = resolver.resolve("wf") assert [s["id"] for s in definition.steps] == ["a", "new", "b"] - def test_resolve_with_installed_overlay(self, project_dir): - data = { - "schema_version": "1.0", - "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, - "steps": [{"id": "a", "type": "command", "command": "speckit.specify"}], - } - _write_workflow(project_dir, "wf", data) - installed_dir = project_dir / ".specify" / "workflows" / "wf" / "overlays" - installed_dir.mkdir(parents=True, exist_ok=True) - installed_dir.joinpath("shipped.yml").write_text( - yaml.safe_dump( - { - "id": "shipped", - "extends": "wf", - "priority": 5, - "edits": [ - { - "operation": "insert_after", - "anchor": "a", - "step": {"id": "shipped-new", "type": "command", "command": "echo"}, - } - ], - } - ), - encoding="utf-8", - ) - - resolver = WorkflowResolver(project_dir) - definition = resolver.resolve("wf") - assert [s["id"] for s in definition.steps] == ["a", "shipped-new"] - def test_resolve_higher_priority_wins(self, project_dir): data = { "schema_version": "1.0", @@ -156,56 +125,6 @@ def test_resolve_higher_priority_wins(self, project_dir): # ends up closer to the anchor and wins the conflict. assert [s["id"] for s in definition.steps] == ["a", "high-step", "low-step"] - def test_resolve_project_wins_tie_with_installed(self, project_dir): - data = { - "schema_version": "1.0", - "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, - "steps": [{"id": "a", "type": "command", "command": "speckit.specify"}], - } - _write_workflow(project_dir, "wf", data) - installed_dir = project_dir / ".specify" / "workflows" / "wf" / "overlays" - installed_dir.mkdir(parents=True, exist_ok=True) - installed_dir.joinpath("shipped.yml").write_text( - yaml.safe_dump( - { - "id": "shipped", - "extends": "wf", - "priority": 10, - "edits": [ - { - "operation": "insert_after", - "anchor": "a", - "step": {"id": "installed-step", "type": "command", "command": "echo"}, - } - ], - } - ), - encoding="utf-8", - ) - _write_overlay( - project_dir, - "wf", - "project", - { - "id": "project", - "extends": "wf", - "priority": 10, - "edits": [ - { - "operation": "insert_after", - "anchor": "a", - "step": {"id": "project-step", "type": "command", "command": "echo"}, - } - ], - }, - ) - - resolver = WorkflowResolver(project_dir) - definition = resolver.resolve("wf") - # Same priority: installed applied first, then project wins tie by - # ending up closer to the anchor. - assert [s["id"] for s in definition.steps] == ["a", "project-step", "installed-step"] - def test_resolve_with_layers_returns_attribution(self, project_dir): data = { "schema_version": "1.0", @@ -237,57 +156,6 @@ def test_resolve_with_layers_returns_attribution(self, project_dir): assert any(layer.tier == "base" for layer in layers) assert attribution == [ComposedStep("a", "base"), ComposedStep("new", "project:ov1")] - def test_resolve_attribution_distinguishes_project_and_installed_same_id(self, project_dir): - data = { - "schema_version": "1.0", - "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, - "steps": [{"id": "a", "type": "command", "command": "speckit.specify"}], - } - _write_workflow(project_dir, "wf", data) - installed_dir = project_dir / ".specify" / "workflows" / "wf" / "overlays" - installed_dir.mkdir(parents=True, exist_ok=True) - installed_dir.joinpath("same.yml").write_text( - yaml.safe_dump( - { - "id": "same", - "extends": "wf", - "priority": 10, - "edits": [ - { - "operation": "insert_after", - "anchor": "a", - "step": {"id": "installed-new", "type": "command", "command": "echo"}, - } - ], - } - ), - encoding="utf-8", - ) - _write_overlay( - project_dir, - "wf", - "same", - { - "id": "same", - "extends": "wf", - "priority": 10, - "edits": [ - { - "operation": "insert_after", - "anchor": "a", - "step": {"id": "project-new", "type": "command", "command": "echo"}, - } - ], - }, - ) - - resolver = WorkflowResolver(project_dir) - definition, _layers, attribution = resolver.resolve_with_layers("wf") - assert [s["id"] for s in definition.steps] == ["a", "project-new", "installed-new"] - sources = {c.step_id: c.source for c in attribution} - assert sources["project-new"] == "project:same" - assert sources["installed-new"] == "installed:same" - def test_resolve_attribution_for_nested_base_steps(self, project_dir): data = { "schema_version": "1.0", @@ -335,31 +203,6 @@ def test_resolve_invalid_project_overlay_fails(self, project_dir): with pytest.raises(ValueError): resolver.resolve("wf") - def test_resolve_invalid_installed_overlay_fails(self, project_dir): - data = { - "schema_version": "1.0", - "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, - "steps": [{"id": "a", "type": "command", "command": "speckit.specify"}], - } - _write_workflow(project_dir, "wf", data) - installed_dir = project_dir / ".specify" / "workflows" / "wf" / "overlays" - installed_dir.mkdir(parents=True, exist_ok=True) - installed_dir.joinpath("broken.yml").write_text( - yaml.safe_dump( - { - "id": "broken", - "extends": "wf", - "priority": 10, - "edits": "not-a-list", - } - ), - encoding="utf-8", - ) - - resolver = WorkflowResolver(project_dir) - with pytest.raises(ValueError): - resolver.resolve("wf") - def test_resolve_disabled_overlay_is_skipped(self, project_dir): data = { "schema_version": "1.0", @@ -493,45 +336,6 @@ def test_resolve_rejects_symlinked_project_overlay_dir(self, project_dir, tmp_pa with pytest.raises(ValueError, match="Symlinked overlay directories are not allowed"): resolver.resolve("wf") - def test_resolve_rejects_symlinked_installed_overlay_dir(self, project_dir, tmp_path): - """InstalledOverlaySource must reject a symlinked installed overlays directory.""" - data = { - "schema_version": "1.0", - "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, - "steps": [{"id": "a", "type": "command", "command": "speckit.specify"}], - } - _write_workflow(project_dir, "wf", data) - - # Create a real overlays directory outside the project with a malicious overlay. - outside_dir = tmp_path / "outside_installed" - outside_dir.mkdir(parents=True, exist_ok=True) - outside_dir.joinpath("evil.yml").write_text( - yaml.safe_dump( - { - "id": "evil", - "extends": "wf", - "priority": 100, - "edits": [ - { - "operation": "insert_after", - "anchor": "a", - "step": {"id": "evil-step", "type": "command", "command": "rm -rf /"}, - } - ], - } - ), - encoding="utf-8", - ) - - # Symlink the installed overlays directory to the outside location. - wf_dir = project_dir / ".specify" / "workflows" / "wf" - symlink_dir = wf_dir / "overlays" - symlink_dir.symlink_to(outside_dir) - - resolver = WorkflowResolver(project_dir) - with pytest.raises(ValueError, match="Symlinked overlay directories are not allowed"): - resolver.resolve("wf") - def test_resolve_attribution_for_inserted_composite_step(self, project_dir): """Inserted composite steps must attribute nested children to the overlay source.""" data = { From d51241bad545edba8e16e73761f51ea8a376f51a Mon Sep 17 00:00:00 2001 From: Markus Date: Fri, 17 Jul 2026 07:45:38 +0200 Subject: [PATCH 05/29] fix(workflows): harden overlay symlink handling Assisted-by: GitHub Copilot (model: GPT-5.4, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/workflows/_commands.py | 2 +- .../workflows/overlays/_commands.py | 25 ++++++- .../workflows/overlays/layer_sources.py | 46 ++++++++++++- tests/test_workflows.py | 32 ++++++++- tests/workflows/test_overlay_security.py | 65 +++++++++++++++++++ tests/workflows/test_resolver_integration.py | 53 +++++++++++++++ 6 files changed, 218 insertions(+), 5 deletions(-) diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index 994952aec6..3e62251e4e 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -116,7 +116,7 @@ def _reject_unsafe_workflow_storage(project_root: Path) -> None: _WORKFLOW_ID_PATTERN = re.compile(r"^[a-z0-9](?:[a-z0-9-]*[a-z0-9])?$") -_RESERVED_WORKFLOW_IDS: frozenset[str] = frozenset({"runs", "steps"}) +_RESERVED_WORKFLOW_IDS: frozenset[str] = frozenset({"overlays", "runs", "steps"}) def _validate_workflow_id_or_exit(workflow_id: str) -> None: diff --git a/src/specify_cli/workflows/overlays/_commands.py b/src/specify_cli/workflows/overlays/_commands.py index 04a5931f3f..ad3b0a517d 100644 --- a/src/specify_cli/workflows/overlays/_commands.py +++ b/src/specify_cli/workflows/overlays/_commands.py @@ -9,6 +9,7 @@ import yaml from ..._console import console, err_console +from .._commands import _reject_unsafe_dir, _reject_unsafe_workflow_storage from . import WorkflowResolver from .schema import _RESERVED_OVERLAY_WORKFLOW_IDS, _SAFE_ID_PATTERN, validate_overlay_yaml @@ -38,8 +39,11 @@ def _validate_workflow_id_or_exit(workflow_id: str) -> None: def _overlay_root(project_root: Path) -> Path: - """Return the project-local overlay root directory.""" - return project_root / ".specify" / "workflows" / "overlays" + """Return the project-local overlay root after rejecting unsafe ancestors.""" + _reject_unsafe_workflow_storage(project_root) + root = project_root / ".specify" / "workflows" / "overlays" + _reject_unsafe_dir(root, ".specify/workflows/overlays") + return root def _project_overlay_dir(project_root: Path, workflow_id: str) -> Path: @@ -58,11 +62,17 @@ def _ensure_contained_dir(path: Path, root: Path) -> Path: Returns *path* if safe. Raises typer.Exit on traversal or symlink. """ + _reject_unsafe_dir(root, ".specify/workflows/overlays") if path.is_symlink(): err_console.print( f"[red]Error:[/red] Refusing to use symlinked path {path}." ) raise typer.Exit(1) + if path.exists() and not path.is_dir(): + err_console.print( + f"[red]Error:[/red] Overlay directory path is not a directory: {path}." + ) + raise typer.Exit(1) try: resolved = path.resolve() root_resolved = root.resolve() @@ -95,6 +105,12 @@ def _find_overlay_file(project_root: Path, workflow_id: str, overlay_id: str) -> def _ensure_contained_path(path: Path, root: Path) -> Path: """Return *path* only if it resolves inside *root*; otherwise raise typer.Exit.""" + _reject_unsafe_dir(root, ".specify/workflows/overlays") + if path.is_symlink(): + err_console.print( + f"[red]Error:[/red] Refusing to use symlinked path {path}." + ) + raise typer.Exit(1) try: resolved = path.resolve() root_resolved = root.resolve() @@ -141,6 +157,7 @@ def workflow_overlay_add( Returns the path of the installed overlay file, or None on failure. """ + _reject_unsafe_workflow_storage(project_root) data, errors = _read_overlay(source) if data is None: for err in errors: @@ -186,6 +203,7 @@ def _update_overlay_field( value: Any, ) -> bool: """Update a single field in a project-local overlay file.""" + _reject_unsafe_workflow_storage(project_root) path = _find_overlay_file(project_root, workflow_id, overlay_id) if path is None: err_console.print( @@ -262,6 +280,7 @@ def workflow_overlay_remove( overlay_id: str, ) -> bool: """Remove a project-local overlay file.""" + _reject_unsafe_workflow_storage(project_root) path = _find_overlay_file(project_root, workflow_id, overlay_id) if path is None: err_console.print( @@ -284,6 +303,7 @@ def workflow_overlay_list(project_root: Path, workflow_id: str) -> list[dict[str Returns the raw list data for machine-readable callers, or None on error. """ + _reject_unsafe_workflow_storage(project_root) _validate_workflow_id_or_exit(workflow_id) resolver = WorkflowResolver(project_root) try: @@ -322,6 +342,7 @@ def workflow_resolve(project_root: Path, workflow_id: str) -> dict[str, Any] | N Returns a serializable attribution payload. """ + _reject_unsafe_workflow_storage(project_root) _validate_workflow_id_or_exit(workflow_id) resolver = WorkflowResolver(project_root) try: diff --git a/src/specify_cli/workflows/overlays/layer_sources.py b/src/specify_cli/workflows/overlays/layer_sources.py index 98a69c8234..9ce64177ca 100644 --- a/src/specify_cli/workflows/overlays/layer_sources.py +++ b/src/specify_cli/workflows/overlays/layer_sources.py @@ -30,6 +30,41 @@ def __init__(self, path: Path, errors: list[str]) -> None: super().__init__(f"Invalid overlay {path}:\n - " + "\n - ".join(errors)) +def _resolve_project_overlay_root(project_root: Path) -> Path: + """Return the unresolved overlay root after rejecting symlinked ancestors. + + This mirrors the older workflow step-directory hardening pattern locally. + The path-walk / containment logic should be centralized in a shared helper + in a follow-up DRY pass, but this stays inline here to keep the Cluster 1 + security fix small and low-risk. + """ + project_root_resolved = project_root.resolve() + overlays_root = project_root / ".specify" / "workflows" / "overlays" + + current = project_root + for part in (".specify", "workflows", "overlays"): + current = current / part + if current.is_symlink(): + raise OverlayLoadError( + current, + [f"Symlinked overlay directories are not allowed ({current})"], + ) + if current.exists() and not current.is_dir(): + raise OverlayLoadError( + current, + [f"Overlay directory path is not a directory ({current})"], + ) + + try: + overlays_root.resolve().relative_to(project_root_resolved) + except ValueError: + raise OverlayLoadError( + overlays_root, + ["Overlay directory escapes the project root"], + ) from None + return overlays_root + + class ProjectOverlaySource: """Project-local overlays: ``.specify/workflows/overlays//*.yml``.""" @@ -41,12 +76,18 @@ def __init__(self, project_root: Path) -> None: def collect(self, workflow_id: str) -> list[Layer]: """Collect all project-local overlays for the given workflow id.""" + self.overlays_dir = _resolve_project_overlay_root(self.project_root) workflow_overlay_dir = self.overlays_dir / workflow_id if workflow_overlay_dir.is_symlink(): raise OverlayLoadError( workflow_overlay_dir, ["Symlinked overlay directories are not allowed"], ) + if workflow_overlay_dir.exists() and not workflow_overlay_dir.is_dir(): + raise OverlayLoadError( + workflow_overlay_dir, + ["Overlay directory path is not a directory"], + ) if not workflow_overlay_dir.is_dir(): return [] layers: list[Layer] = [] @@ -55,7 +96,10 @@ def collect(self, workflow_id: str) -> list[Layer]: continue if path.is_symlink(): raise OverlayLoadError(path, ["Symlinked overlay files are not allowed"]) - data = yaml.safe_load(path.read_text(encoding="utf-8")) or {} + try: + data = yaml.safe_load(path.read_text(encoding="utf-8")) or {} + except yaml.YAMLError as exc: + raise OverlayLoadError(path, [f"Invalid YAML: {exc}"]) from exc overlay, errors = validate_overlay_yaml(data) if overlay is None or errors: raise OverlayLoadError(path, errors) diff --git a/tests/test_workflows.py b/tests/test_workflows.py index d7cff20f6d..49a177d0f8 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -5786,7 +5786,7 @@ def test_remove_rejects_traversal_registry_key(self, project_dir, monkeypatch): assert "Invalid workflow ID" in result.output assert sentinel.read_text(encoding="utf-8") == "keep" - @pytest.mark.parametrize("workflow_id", ["runs", "steps"]) + @pytest.mark.parametrize("workflow_id", ["overlays", "runs", "steps"]) def test_remove_rejects_reserved_storage_ids( self, project_dir, monkeypatch, workflow_id ): @@ -6007,9 +6007,39 @@ def test_safe_workflow_id_dir_escapes_markup_in_invalid_id(self, temp_dir, capsy # Literal bracketed text survives; Rich did not consume it as a tag. assert "[red]evil[/red]" in out + def test_add_rejects_reserved_overlay_storage_id(self, temp_dir, monkeypatch): + """workflow add must not install into the overlay storage directory.""" + from typer.testing import CliRunner + from specify_cli import app + + (temp_dir / ".specify" / "workflows").mkdir(parents=True) + overlay_file = temp_dir / "incoming.yml" + overlay_file.write_text( + """ +schema_version: "1.0" +workflow: + id: "overlays" + name: "Bad Workflow" + version: "1.0.0" +steps: + - id: step-one + command: speckit.specify +""".strip() + + "\n", + encoding="utf-8", + ) + + monkeypatch.chdir(temp_dir) + result = CliRunner().invoke(app, ["workflow", "add", str(overlay_file)]) + + assert result.exit_code != 0 + assert "Invalid workflow ID" in result.output + assert not (temp_dir / ".specify" / "workflows" / "overlays" / "workflow.yml").exists() + @pytest.mark.parametrize( "workflow_id", [ + "overlays", "runs", "steps", "nested/workflow", diff --git a/tests/workflows/test_overlay_security.py b/tests/workflows/test_overlay_security.py index 2d392cd1bc..505b438d2b 100644 --- a/tests/workflows/test_overlay_security.py +++ b/tests/workflows/test_overlay_security.py @@ -165,6 +165,50 @@ def test_overlay_remove_rejects_symlink(self, project_dir, monkeypatch): assert real_file.is_file() assert "symlink" in result.output.lower() or "Invalid" in result.output + def test_overlay_add_rejects_symlinked_target_file(self, project_dir, monkeypatch): + """overlay add must not overwrite through a symlinked overlay file target.""" + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + _write_workflow( + project_dir, + "wf", + { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "echo"}], + }, + ) + + overlay_dir = project_dir / ".specify" / "workflows" / "overlays" / "wf" + overlay_dir.mkdir(parents=True, exist_ok=True) + real_file = overlay_dir / "other.yml" + real_file.write_text("sentinel\n", encoding="utf-8") + (overlay_dir / "ov1.yml").symlink_to(real_file) + + overlay_file = project_dir / "overlay.yml" + overlay_file.write_text( + yaml.safe_dump( + { + "id": "ov1", + "extends": "wf", + "priority": 10, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "new", "type": "command", "command": "echo"}, + } + ], + } + ), + encoding="utf-8", + ) + + result = runner.invoke(app, ["workflow", "overlay", "add", str(overlay_file)]) + + assert result.exit_code != 0, result.output + assert "symlinked path" in result.output.lower() + assert real_file.read_text(encoding="utf-8") == "sentinel\n" + def test_overlay_operations_reject_overlays_as_workflow_id(self, project_dir, monkeypatch): monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) result = runner.invoke(app, ["workflow", "overlay", "list", "overlays"]) @@ -251,3 +295,24 @@ def test_overlay_list_rejects_symlinked_per_workflow_dir(self, project_dir, monk result = runner.invoke(app, ["workflow", "overlay", "list", "wf"]) assert result.exit_code != 0, result.output assert "symlink" in result.output.lower() + + def test_overlay_list_reports_invalid_yaml_cleanly(self, project_dir, monkeypatch): + """Overlay list should surface malformed overlay YAML as a clean user error.""" + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + _write_workflow( + project_dir, + "wf", + { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "echo"}], + }, + ) + overlay_dir = project_dir / ".specify" / "workflows" / "overlays" / "wf" + overlay_dir.mkdir(parents=True, exist_ok=True) + (overlay_dir / "broken.yml").write_text("id: broken\nextends: wf\npriority: [\n", encoding="utf-8") + + result = runner.invoke(app, ["workflow", "overlay", "list", "wf"]) + + assert result.exit_code != 0, result.output + assert "Invalid YAML" in result.output diff --git a/tests/workflows/test_resolver_integration.py b/tests/workflows/test_resolver_integration.py index b11fc6bed8..afd3461bd4 100644 --- a/tests/workflows/test_resolver_integration.py +++ b/tests/workflows/test_resolver_integration.py @@ -336,6 +336,59 @@ def test_resolve_rejects_symlinked_project_overlay_dir(self, project_dir, tmp_pa with pytest.raises(ValueError, match="Symlinked overlay directories are not allowed"): resolver.resolve("wf") + def test_resolve_rejects_symlinked_overlay_root(self, project_dir, tmp_path): + """ProjectOverlaySource must reject a symlinked overlay root too.""" + data = { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "speckit.specify"}], + } + _write_workflow(project_dir, "wf", data) + + outside_root = tmp_path / "outside-overlays-root" + outside_root.mkdir(parents=True, exist_ok=True) + workflow_dir = outside_root / "wf" + workflow_dir.mkdir() + workflow_dir.joinpath("evil.yml").write_text( + yaml.safe_dump( + { + "id": "evil", + "extends": "wf", + "priority": 100, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "evil-step", "type": "command", "command": "rm -rf /"}, + } + ], + } + ), + encoding="utf-8", + ) + + overlays_root = project_dir / ".specify" / "workflows" / "overlays" + overlays_root.symlink_to(outside_root, target_is_directory=True) + + resolver = WorkflowResolver(project_dir) + with pytest.raises(ValueError, match="Symlinked overlay directories are not allowed"): + resolver.resolve("wf") + + def test_resolve_reports_invalid_overlay_yaml_cleanly(self, project_dir): + data = { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "speckit.specify"}], + } + _write_workflow(project_dir, "wf", data) + overlay_dir = project_dir / ".specify" / "workflows" / "overlays" / "wf" + overlay_dir.mkdir(parents=True, exist_ok=True) + (overlay_dir / "broken.yml").write_text("id: broken\nextends: wf\npriority: [\n", encoding="utf-8") + + resolver = WorkflowResolver(project_dir) + with pytest.raises(ValueError, match="Invalid YAML"): + resolver.resolve("wf") + def test_resolve_attribution_for_inserted_composite_step(self, project_dir): """Inserted composite steps must attribute nested children to the overlay source.""" data = { From ed0daa76575284996cac26e18d830c2e696edcd4 Mon Sep 17 00:00:00 2001 From: Markus Date: Fri, 17 Jul 2026 07:52:19 +0200 Subject: [PATCH 06/29] docs(workflows): remove stale installed-overlay references from workflows.md The 2-tier refactor (cc28185) removed the installed-overlay tier entirely, but docs/reference/workflows.md was not updated. This commit addresses all four Cluster 2 findings from the PR review: - workflow add: remove sentence about copying overlays/ subdirectory - How Overlays Work: drop installed-overlay table row and precedence prose; rewrite to 2-tier model (project overlays only, source-order tie-break) - overlay remove: drop trailing sentence about installed overlays - Interaction with Bundles: rewrite to say workflow add installs only workflow.yml; remove installed-overlay discovery language Fixes: r3596368791, r3596368831, r3596368873, r3596368919 Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/reference/workflows.md | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index 37e3de7919..8779dae7f6 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -86,7 +86,7 @@ Lists workflows installed in the current project. specify workflow add ``` -Installs a workflow from the catalog, a URL (HTTPS required), a local YAML file, or a local directory containing `workflow.yml`. Local workflow directories may include an optional `overlays/` subdirectory, which is installed alongside the workflow as shipped overlays. +Installs a workflow from the catalog, a URL (HTTPS required), a local YAML file, or a local directory containing `workflow.yml`. ## Workflow Overlays @@ -96,16 +96,13 @@ When `specify workflow run ` loads a workflow, the engine composes ### How Overlays Work -An overlay is a YAML file that declares a set of edit operations against the step list of a base workflow. Overlays are applied in priority order (lowest first, highest last); at the same priority, installed overlays are applied before project overlays, so project overlays win ties. +An overlay is a YAML file that declares a set of edit operations against the step list of a base workflow. Overlays are applied in priority order (lowest first, highest last); equal-priority overlays are applied in source order (last applied wins). -Overlay files live in two locations: +Project overlay files live at: -| Location | Purpose | Tier | -| --- | --- | --- | -| `.specify/workflows//overlays/*.yml` | Shipped with the installed workflow | `installed-overlay` | -| `.specify/workflows/overlays//*.yml` | Project-local customizations | `project-overlay` | - -Project overlays always take precedence over installed overlays at the same priority. +| Location | Purpose | +| --- | --- | +| `.specify/workflows/overlays//*.yml` | Project-local customizations | ### Overlay File Format @@ -204,7 +201,7 @@ specify workflow overlay enable specify workflow overlay remove ``` -Removes the project overlay file. Installed overlays shipped with a workflow are removed by `specify workflow remove `, which deletes the entire workflow directory. +Removes the project overlay file. #### Inspect the Composed Workflow @@ -264,7 +261,7 @@ Higher priority (`20`) means this overlay is applied after the `add-lint` overla ### Interaction with Bundles and Updates -`specify workflow add ` copies `workflow.yml` and an optional `overlays/` subdirectory into `.specify/workflows//`. Overlays shipped this way are discovered automatically as `installed-overlay` layers. +`specify workflow add ` installs `workflow.yml` from the local directory into `.specify/workflows//`. When an installed workflow is refreshed or reinstalled, project overlays in `.specify/workflows/overlays//` are preserved because they live outside the installed workflow directory. From b890fddbec47aa111a840347a58b4b33ca5da904 Mon Sep 17 00:00:00 2001 From: Markus Date: Fri, 17 Jul 2026 09:56:10 +0200 Subject: [PATCH 07/29] fix(overlays): detect ancestor-conflict anchors in merge_steps When two overlay edits target anchors that share a parent/descendant relationship (e.g. remove an if-step + insert_after a nested child), merge_steps processed them independently and in dict-insertion order, making the outcome non-deterministic. Add two private helpers to merge.py: - _descendant_ids(step): returns all step IDs nested inside a step dict by delegating to the existing _all_base_step_ids helper on children. - _check_anchor_conflicts(anchors, base_steps): for each targeted anchor finds its descendants and checks whether any other targeted anchor is among them; returns human-readable error strings. Wire _check_anchor_conflicts into merge_steps immediately after edits_by_anchor is built, before any tree mutation occurs. Raises ValueError listing the conflicting anchor pair(s) so overlay authors know exactly what to fix. Add TestMergeStepsAncestorConflicts (6 cases): - remove parent + insert_after child raises ValueError - replace parent + remove child raises ValueError - conflict across multiple overlays raises ValueError - sibling anchors (not ancestor/descendant) pass - single anchor passes - parent targeted but child not targeted passes Closes review comment r3596368746 (PR #3557, round 2, cluster 3). Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/workflows/overlays/merge.py | 51 ++++++++ tests/workflows/test_overlay_merge.py | 128 ++++++++++++++++++++ 2 files changed, 179 insertions(+) diff --git a/src/specify_cli/workflows/overlays/merge.py b/src/specify_cli/workflows/overlays/merge.py index 3b11cd1e0c..b9cdf996ab 100644 --- a/src/specify_cli/workflows/overlays/merge.py +++ b/src/specify_cli/workflows/overlays/merge.py @@ -81,6 +81,49 @@ def _all_base_step_ids(steps: list[dict[str, Any]]) -> set[str]: return ids +def _descendant_ids(step: dict[str, Any]) -> set[str]: + """Return all step IDs nested inside *step* (not including *step* itself).""" + ids: set[str] = set() + for key in _NESTED_LIST_KEYS: + nested = step.get(key) + if isinstance(nested, list): + ids.update(_all_base_step_ids(nested)) + cases = step.get("cases") + if isinstance(cases, dict): + for case_steps in cases.values(): + if isinstance(case_steps, list): + ids.update(_all_base_step_ids(case_steps)) + return ids + + +def _check_anchor_conflicts( + anchors: set[str], + base_steps: list[dict[str, Any]], +) -> list[str]: + """Return error messages for anchor pairs where one is an ancestor of the other. + + When two targeted anchors share a parent/descendant relationship the edit + processing order determines success or failure, producing non-deterministic + results. Callers should raise on any returned errors before mutating the + step tree. + """ + errors: list[str] = [] + for anchor in sorted(anchors): # sorted for deterministic error order + location = find_step(base_steps, anchor) + if location is None: + continue # missing anchors are reported by validate_edits + parent_list, idx = location + step = parent_list[idx] + conflicting = anchors & _descendant_ids(step) + for child_anchor in sorted(conflicting): + errors.append( + f"Anchor conflict: '{anchor}' is an ancestor of '{child_anchor}'. " + "Targeting both anchors in the same overlay set produces " + "order-dependent results; restructure edits to avoid nesting." + ) + return errors + + def _init_sources_recursively( steps: list[dict[str, Any]], sources: dict[str, str] ) -> None: @@ -252,6 +295,14 @@ def merge_steps( for edit in layer.overlay.edits: edits_by_anchor.setdefault(edit.anchor, []).append((layer, edit)) + # Reject edits that target anchors with a parent/descendant relationship — + # processing such pairs independently produces order-dependent results. + anchor_conflicts = _check_anchor_conflicts(set(edits_by_anchor.keys()), base_steps) + if anchor_conflicts: + raise ValueError( + "Overlay anchor conflict(s) detected:\n - " + "\n - ".join(anchor_conflicts) + ) + for anchor, edits in edits_by_anchor.items(): # Highest-priority edit is last because *overlays* is already sorted. _, winning_edit = edits[-1] diff --git a/tests/workflows/test_overlay_merge.py b/tests/workflows/test_overlay_merge.py index b45bc73767..4ef4bf8b9b 100644 --- a/tests/workflows/test_overlay_merge.py +++ b/tests/workflows/test_overlay_merge.py @@ -579,3 +579,131 @@ def test_remove_requires_no_step(self): edits = [OverlayEdit("remove", "a", _step("extra"))] errors = validate_edits(edits, {"a"}) assert len(errors) > 0 + + +class TestMergeStepsAncestorConflicts: + """merge_steps raises when two targeted anchors are in a parent/descendant relationship.""" + + def _if_step(self, parent_id: str, child_id: str) -> dict[str, Any]: + return { + "id": parent_id, + "type": "if", + "condition": "true", + "then": [_step(child_id)], + } + + def test_remove_parent_and_insert_after_child_raises(self): + """Removing a parent while inserting after its nested child is an anchor conflict.""" + parent_id = "if-step" + child_id = "then-child" + base = [self._if_step(parent_id, child_id)] + overlay = Overlay( + id="ov", + extends="wf", + priority=10, + edits=[ + OverlayEdit("remove", parent_id), + OverlayEdit("insert_after", child_id, _step("new-step")), + ], + ) + with pytest.raises(ValueError, match="ancestor"): + merge_steps(base, [_layer(overlay, "project:ov")]) + + def test_replace_parent_and_remove_child_raises(self): + """Replacing a parent while also removing a nested child is an anchor conflict.""" + parent_id = "if-step" + child_id = "then-child" + base = [self._if_step(parent_id, child_id)] + overlay = Overlay( + id="ov", + extends="wf", + priority=10, + edits=[ + OverlayEdit("replace", parent_id, _step("new-parent")), + OverlayEdit("remove", child_id), + ], + ) + with pytest.raises(ValueError, match="ancestor"): + merge_steps(base, [_layer(overlay, "project:ov")]) + + def test_conflict_across_multiple_overlays_raises(self): + """Conflict is detected even when conflicting anchors come from different overlays.""" + parent_id = "if-step" + child_id = "then-child" + base = [self._if_step(parent_id, child_id)] + overlay_a = Overlay( + id="ov-a", + extends="wf", + priority=5, + edits=[OverlayEdit("remove", parent_id)], + ) + overlay_b = Overlay( + id="ov-b", + extends="wf", + priority=10, + edits=[OverlayEdit("insert_after", child_id, _step("new-step"))], + ) + with pytest.raises(ValueError, match="ancestor"): + merge_steps( + base, + [_layer(overlay_a, "project:ov-a"), _layer(overlay_b, "project:ov-b")], + ) + + def test_sibling_anchors_not_conflicting(self): + """Anchors in sibling branches (not ancestor/descendant) are allowed.""" + base = [ + { + "id": "if-step", + "type": "if", + "condition": "true", + "then": [_step("then-child")], + "else": [_step("else-child")], + } + ] + overlay = Overlay( + id="ov", + extends="wf", + priority=10, + edits=[ + OverlayEdit("insert_after", "then-child", _step("after-then")), + OverlayEdit("insert_after", "else-child", _step("after-else")), + ], + ) + # Should not raise — the two anchors are siblings, not ancestor/descendant. + steps, _ = merge_steps(base, [_layer(overlay, "project:ov")]) + step_ids = [s.get("id") for s in steps[0]["then"]] + [s.get("id") for s in steps[0]["else"]] + assert "after-then" in step_ids + assert "after-else" in step_ids + + def test_single_anchor_not_conflicting(self): + """A single anchor is never in conflict with itself.""" + parent_id = "if-step" + child_id = "then-child" + base = [self._if_step(parent_id, child_id)] + overlay = Overlay( + id="ov", + extends="wf", + priority=10, + edits=[OverlayEdit("remove", parent_id)], + ) + steps, _ = merge_steps(base, [_layer(overlay, "project:ov")]) + assert steps == [] + + def test_child_not_targeted_no_conflict(self): + """Targeting a parent alone (child not in any edit) is allowed.""" + parent_id = "if-step" + child_id = "then-child" + base = [self._if_step(parent_id, child_id), _step("other")] + overlay = Overlay( + id="ov", + extends="wf", + priority=10, + edits=[ + OverlayEdit("remove", parent_id), + OverlayEdit("insert_after", "other", _step("new-step")), + ], + ) + # "other" is not inside "if-step", so no ancestor conflict. + steps, _ = merge_steps(base, [_layer(overlay, "project:ov")]) + assert [s["id"] for s in steps] == ["other", "new-step"] + From 586500f3bb6f14df8f50eb0f6d133448a58986b6 Mon Sep 17 00:00:00 2001 From: Markus Date: Fri, 17 Jul 2026 16:30:19 +0200 Subject: [PATCH 08/29] fix(overlays): fix over-broad conflict detection and non-deterministic ID collision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finding 1.1 — _check_anchor_conflicts was rejecting any ancestor/descendant anchor pair, including insert-only edits that are perfectly safe. Only replace/remove on an ancestor can destroy its subtree and make a descendant anchor unresolvable. Change the signature to accept a dict[str, str] (anchor → winning operation) and skip the check for insert_after/insert_before. Finding 1.2 — merge_steps was calling find_step on the already-mutated tree, so a replacement step that reused a base step ID could be accidentally targeted by a later edit group (non-deterministic result depending on dict iteration order). Replace the anchor-group loop with a single-pass _traverse_and_apply that walks the original tree structure and applies edits as each step is encountered. Anchors are never re-looked up in a mutated tree. Design invariant enforced: overlays always apply to the original base tree and cannot target steps introduced by other overlays. Non-remove edits on non-base anchors now raise ValueError early. Also removes apply_edit (no production callers, only tested in isolation) and its test class — the new traversal inlines the same mechanics without the find_step round-trip. Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/workflows/overlays/merge.py | 198 ++++++++++---------- tests/workflows/test_overlay_merge.py | 113 +++++------ 2 files changed, 161 insertions(+), 150 deletions(-) diff --git a/src/specify_cli/workflows/overlays/merge.py b/src/specify_cli/workflows/overlays/merge.py index b9cdf996ab..a1a6fdd607 100644 --- a/src/specify_cli/workflows/overlays/merge.py +++ b/src/specify_cli/workflows/overlays/merge.py @@ -97,24 +97,29 @@ def _descendant_ids(step: dict[str, Any]) -> set[str]: def _check_anchor_conflicts( - anchors: set[str], + anchor_operations: dict[str, str], base_steps: list[dict[str, Any]], ) -> list[str]: """Return error messages for anchor pairs where one is an ancestor of the other. - When two targeted anchors share a parent/descendant relationship the edit - processing order determines success or failure, producing non-deterministic - results. Callers should raise on any returned errors before mutating the - step tree. + Only flags conflicts where the ancestor's winning edit is ``replace`` or + ``remove`` — operations that destroy the subtree and make any descendant + anchor unresolvable. Pure insert operations on an ancestor leave it intact, + so its descendants remain reachable regardless of processing order. + + Callers should raise on any returned errors before mutating the step tree. """ errors: list[str] = [] - for anchor in sorted(anchors): # sorted for deterministic error order + for anchor, operation in sorted(anchor_operations.items()): + if operation in ("insert_after", "insert_before"): + # Inserts leave the ancestor step intact; descendants are unaffected. + continue location = find_step(base_steps, anchor) if location is None: continue # missing anchors are reported by validate_edits parent_list, idx = location step = parent_list[idx] - conflicting = anchors & _descendant_ids(step) + conflicting = set(anchor_operations.keys()) & _descendant_ids(step) for child_anchor in sorted(conflicting): errors.append( f"Anchor conflict: '{anchor}' is an ancestor of '{child_anchor}'. " @@ -200,51 +205,6 @@ def _remove_sources_recursively( _remove_sources_recursively(child, sources) -def apply_edit( - steps: list[dict[str, Any]], - edit: OverlayEdit, - source: str, -) -> tuple[list[dict[str, Any]], ComposedStep | None, str | None]: - """Apply a single edit to a step list and return the mutated list. - - The returned list is the same list object as *steps* but deep-copied first - so callers can avoid mutating the base. Raises ``ValueError`` if the anchor - is not found. - - Returns: - ``(steps, composed_step, replaced_or_removed_id)``. *composed_step* is - ``None`` for ``remove`` operations; *replaced_or_removed_id* is the - previous step id for ``replace``/``remove`` operations, otherwise - ``None``. - """ - location = find_step(steps, edit.anchor) - if location is None: - raise ValueError(f"Anchor '{edit.anchor}' not found in workflow steps.") - - parent_list, index = location - old_step = parent_list[index] - old_step_id = old_step.get("id") if isinstance(old_step, dict) else None - - if edit.operation == "insert_after": - new_step = copy.deepcopy(edit.step) - parent_list.insert(index + 1, new_step) - step_id = str(new_step.get("id")) if isinstance(new_step, dict) else "" - return steps, ComposedStep(step_id, source), None - if edit.operation == "insert_before": - new_step = copy.deepcopy(edit.step) - parent_list.insert(index, new_step) - step_id = str(new_step.get("id")) if isinstance(new_step, dict) else "" - return steps, ComposedStep(step_id, source), None - if edit.operation == "replace": - new_step = copy.deepcopy(edit.step) - parent_list[index] = new_step - step_id = str(new_step.get("id")) if isinstance(new_step, dict) else "" - return steps, ComposedStep(step_id, source), old_step_id - if edit.operation == "remove": - del parent_list[index] - return steps, None, old_step_id - raise ValueError(f"Unsupported edit operation: {edit.operation}") - def _build_attribution( steps: list[dict[str, Any]], @@ -270,6 +230,74 @@ def _build_attribution( return result +def _traverse_and_apply( + steps: list[dict[str, Any]], + edits_by_anchor: dict[str, list[tuple[OverlayLayer, OverlayEdit]]], + sources: dict[str, str], +) -> list[dict[str, Any]]: + """Walk the original step tree and apply overlay edits as each step is encountered. + + Edits are always resolved against the *original* structure — this function + traverses the unmodified list passed in, so a replacement step's new ID can + never be mistaken for a base anchor. Nested lists (``then``, ``else``, etc.) + are recursed into only for steps that survive the edit (not for replaced + steps). + + *edits* are expected to be in merge order (lowest priority first, highest + priority last); the winning edit for each anchor is ``edits[-1]``. + """ + result: list[dict[str, Any]] = [] + + for step in steps: + if not isinstance(step, dict): + result.append(step) + continue + + step_id = step.get("id") + edits = edits_by_anchor.get(step_id, []) if isinstance(step_id, str) else [] + winning_edit = edits[-1][1] if edits else None + + if winning_edit is not None and winning_edit.operation == "remove": + # Winning edit removes this step; ignore all other edits on this anchor. + _remove_sources_recursively(step, sources) + continue + + # Insert before (in merge order). + for layer, edit in edits: + if edit.operation == "insert_before": + new_step = copy.deepcopy(edit.step) + _record_sources_recursively(new_step, layer.source, sources) + result.append(new_step) + + if winning_edit is not None and winning_edit.operation == "replace": + winning_layer = edits[-1][0] + new_step = copy.deepcopy(winning_edit.step) + _remove_sources_recursively(step, sources) + _record_sources_recursively(new_step, winning_layer.source, sources) + result.append(new_step) + else: + # No replacement: keep this step and recurse into its nested lists. + for key in _NESTED_LIST_KEYS: + nested = step.get(key) + if isinstance(nested, list): + step[key] = _traverse_and_apply(nested, edits_by_anchor, sources) + cases = step.get("cases") + if isinstance(cases, dict): + for case_key, case_steps in cases.items(): + if isinstance(case_steps, list): + cases[case_key] = _traverse_and_apply(case_steps, edits_by_anchor, sources) + result.append(step) + + # Insert after (highest priority closest to anchor — reversed merge order). + for layer, edit in reversed(edits): + if edit.operation == "insert_after": + new_step = copy.deepcopy(edit.step) + _record_sources_recursively(new_step, layer.source, sources) + result.append(new_step) + + return result + + def merge_steps( base_steps: list[dict[str, Any]], overlays: list[OverlayLayer], @@ -295,54 +323,36 @@ def merge_steps( for edit in layer.overlay.edits: edits_by_anchor.setdefault(edit.anchor, []).append((layer, edit)) - # Reject edits that target anchors with a parent/descendant relationship — - # processing such pairs independently produces order-dependent results. - anchor_conflicts = _check_anchor_conflicts(set(edits_by_anchor.keys()), base_steps) + # Raise early for non-remove edits that target anchors not present in the base. + # Overlays always apply to the original tree; they cannot target steps introduced + # by other overlays. + base_ids = _all_base_step_ids(base_steps) + for anchor, anchor_edits in edits_by_anchor.items(): + winning_op = anchor_edits[-1][1].operation + if winning_op != "remove" and anchor not in base_ids: + raise ValueError(f"Anchor '{anchor}' not found in workflow steps.") + + # Reject edits that target anchors with a parent/descendant relationship when + # the ancestor edit replaces or removes its subtree — those produce + # order-dependent results. Pure insert edits on an ancestor are safe because + # the ancestor step (and its descendants) remain intact. + anchor_winning_ops = { + anchor: anchor_edits[-1][1].operation + for anchor, anchor_edits in edits_by_anchor.items() + } + anchor_conflicts = _check_anchor_conflicts(anchor_winning_ops, base_steps) if anchor_conflicts: raise ValueError( "Overlay anchor conflict(s) detected:\n - " + "\n - ".join(anchor_conflicts) ) - for anchor, edits in edits_by_anchor.items(): - # Highest-priority edit is last because *overlays* is already sorted. - _, winning_edit = edits[-1] - - if winning_edit.operation == "remove": - # Anchor is removed; ignore all other edits on this anchor. - location = find_step(steps, anchor) - if location is not None: - parent_list, index = location - removed_step = parent_list[index] - del parent_list[index] - if isinstance(removed_step, dict): - _remove_sources_recursively(removed_step, sources) - continue + # Apply all overlay edits via a single-pass traversal of the original tree. + # Each edit is resolved against the original step structure, so a replacement + # step's new ID can never be mistaken for a base anchor in a later edit group. + result = _traverse_and_apply(steps, edits_by_anchor, sources) - # For replace/insert_*, the anchor survives. Only the highest-priority - # replace is applied; lower-priority replaces on the same anchor are - # skipped. Inserts are applied in merge order. - # - # Inserts must be applied *before* the winning replace: if the replace - # changes the step ID, ``find_step`` can no longer locate the original - # anchor and the inserts would raise. - for layer, edit in edits: - if edit.operation in ("insert_after", "insert_before"): - steps, composed, replaced_id = apply_edit(steps, edit, layer.source) - if replaced_id is not None: - sources.pop(replaced_id, None) - if composed is not None: - _record_sources_recursively(edit.step, composed.source, sources) - - if winning_edit.operation == "replace": - winning_layer, _ = edits[-1] - steps, composed, replaced_id = apply_edit(steps, winning_edit, winning_layer.source) - if isinstance(replaced_id, str): - sources.pop(replaced_id, None) - if composed is not None: - _record_sources_recursively(winning_edit.step, composed.source, sources) - - attribution = _build_attribution(steps, sources) - return steps, attribution + attribution = _build_attribution(result, sources) + return result, attribution def validate_edits( diff --git a/tests/workflows/test_overlay_merge.py b/tests/workflows/test_overlay_merge.py index 4ef4bf8b9b..b699b92c31 100644 --- a/tests/workflows/test_overlay_merge.py +++ b/tests/workflows/test_overlay_merge.py @@ -10,7 +10,6 @@ from specify_cli.workflows.overlays.merge import ( ComposedStep, OverlayLayer, - apply_edit, find_step, merge_steps, validate_edits, @@ -114,59 +113,6 @@ def test_find_step_not_in_fan_out_template(self): assert find_step(steps, "template-x") is None -class TestApplyEdit: - """Single edit operations against a step list.""" - - def test_insert_after(self): - steps = [_step("a"), _step("b")] - new_step = _step("new") - result, composed, replaced = apply_edit(steps, OverlayEdit("insert_after", "a", new_step), "project") - assert [s["id"] for s in result] == ["a", "new", "b"] - assert composed == ComposedStep("new", "project") - assert replaced is None - - def test_insert_before(self): - steps = [_step("a"), _step("b")] - new_step = _step("new") - result, composed, replaced = apply_edit(steps, OverlayEdit("insert_before", "b", new_step), "project") - assert [s["id"] for s in result] == ["a", "new", "b"] - assert replaced is None - - def test_replace(self): - steps = [_step("a"), _step("b")] - new_step = _step("replaced") - result, composed, replaced = apply_edit(steps, OverlayEdit("replace", "b", new_step), "installed:x") - assert [s["id"] for s in result] == ["a", "replaced"] - assert composed == ComposedStep("replaced", "installed:x") - assert replaced == "b" - - def test_remove(self): - steps = [_step("a"), _step("b"), _step("c")] - result, composed, replaced = apply_edit(steps, OverlayEdit("remove", "b"), "project") - assert [s["id"] for s in result] == ["a", "c"] - assert composed is None - assert replaced == "b" - - def test_apply_edit_anchors_nested(self): - steps = [ - { - "id": "if-1", - "type": "if", - "condition": "true", - "then": [_step("then-a")], - }, - ] - new_step = _step("new") - result, _, _ = apply_edit(steps, OverlayEdit("insert_after", "then-a", new_step), "project") - assert [s["id"] for s in result[0]["then"]] == ["then-a", "new"] - - def test_apply_edit_missing_anchor(self): - steps = [_step("a")] - new_step = _step("new") - with pytest.raises(ValueError, match="Anchor 'missing' not found"): - apply_edit(steps, OverlayEdit("insert_after", "missing", new_step), "project") - - class TestMergeSteps: """Composition of multiple overlays in merge order.""" @@ -217,6 +163,7 @@ def test_merge_steps_higher_priority_wins(self): ] def test_merge_steps_replace_wins_over_insert(self): + """Overlays apply to the original tree only; targeting an overlay-introduced step raises.""" base = [_step("a")] insert = Overlay( id="insert", @@ -230,8 +177,9 @@ def test_merge_steps_replace_wins_over_insert(self): priority=10, edits=[OverlayEdit("replace", "inserted", _step("replaced"))], ) - steps, _ = merge_steps(base, [_layer(insert, "project:insert"), _layer(replace, "project:replace")]) - assert [s["id"] for s in steps] == ["a", "replaced"] + # "inserted" is not a base step — overlays cannot target each other's steps. + with pytest.raises(ValueError, match="Anchor 'inserted' not found"): + merge_steps(base, [_layer(insert, "project:insert"), _layer(replace, "project:replace")]) def test_merge_steps_does_not_mutate_base(self): base = [_step("a")] @@ -707,3 +655,56 @@ def test_child_not_targeted_no_conflict(self): steps, _ = merge_steps(base, [_layer(overlay, "project:ov")]) assert [s["id"] for s in steps] == ["other", "new-step"] + def test_insert_only_on_ancestor_and_descendant_not_conflicting(self): + """insert_after on both a parent and its nested child is valid and order-independent.""" + parent_id = "if-step" + child_id = "then-child" + base = [self._if_step(parent_id, child_id)] + overlay = Overlay( + id="ov", + extends="wf", + priority=10, + edits=[ + OverlayEdit("insert_after", parent_id, _step("after-parent")), + OverlayEdit("insert_after", child_id, _step("after-child")), + ], + ) + # Should not raise — inserts leave the ancestor intact. + steps, attribution = merge_steps(base, [_layer(overlay, "project:ov")]) + # "after-parent" is inserted at the top level after the if-step. + assert [s["id"] for s in steps] == [parent_id, "after-parent"] + # "after-child" is inserted inside the then list. + then_ids = [s["id"] for s in steps[0]["then"]] + assert then_ids == [child_id, "after-child"] + + +class TestMergeStepsIdCollision: + """merge_steps is deterministic when a replacement reuses a base step ID.""" + + def test_replace_with_reused_id_does_not_affect_original(self): + """Replacing A with new_step(id=B) must not interfere with editing original B. + + Before the fix, the remove-B anchor group would find the replacement step + (which now has id='b') instead of the original 'b' step, producing a + different result depending on dict iteration order. + """ + base = [_step("a"), _step("b"), _step("c")] + overlay = Overlay( + id="ov", + extends="wf", + priority=10, + edits=[ + # Replace "a" with a new step that reuses id "b". + OverlayEdit("replace", "a", {**_step("b"), "command": "speckit.replaced"}), + # Remove the original "b". + OverlayEdit("remove", "b"), + ], + ) + steps, _ = merge_steps(base, [_layer(overlay, "project:ov")]) + # The original "b" is removed; the replacement (also id="b") survives. + # "c" is untouched. + assert len(steps) == 2 + remaining_ids = [s["id"] for s in steps] + assert remaining_ids == ["b", "c"] + # The surviving "b" step is the replacement (has the custom command). + assert steps[0]["command"] == "speckit.replaced" From d9090243521e2a2f52c734bc89cd23573d12435d Mon Sep 17 00:00:00 2001 From: Markus Date: Fri, 17 Jul 2026 16:39:13 +0200 Subject: [PATCH 09/29] fix(overlays): reject ID trailing newlines and reuse existing .yaml path Fix two input validation bugs in the overlay layer (Group 2 of copilot review PR #3557): 1. _validate_safe_id in schema.py used re.match() which anchors only at the start of the string, so IDs like 'overlay\n' passed validation and could produce newline-containing file paths. Changed to fullmatch() so the entire string must satisfy the pattern. 2. workflow_overlay_add always wrote .yml without checking whether .yaml already existed. Since the resolver loads both extensions, this created two active layers whose edits applied twice. Now uses the existing _find_overlay_file() to detect a pre-existing file and reuse its path, falling back to .yml only for new overlays. Tests added for both fixes. Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../workflows/overlays/_commands.py | 11 ++-- src/specify_cli/workflows/overlays/schema.py | 2 +- tests/workflows/test_overlay_commands.py | 52 +++++++++++++++++++ tests/workflows/test_overlay_schema.py | 26 ++++++++++ 4 files changed, 87 insertions(+), 4 deletions(-) diff --git a/src/specify_cli/workflows/overlays/_commands.py b/src/specify_cli/workflows/overlays/_commands.py index ad3b0a517d..c17450261a 100644 --- a/src/specify_cli/workflows/overlays/_commands.py +++ b/src/specify_cli/workflows/overlays/_commands.py @@ -179,9 +179,14 @@ def workflow_overlay_add( target_dir = _project_overlay_dir(project_root, overlay.extends) target_dir.mkdir(parents=True, exist_ok=True) - target_path = _ensure_contained_path( - target_dir / f"{overlay.id}.yml", _overlay_root(project_root) - ) + # Reuse an existing .yaml file so we don't create a duplicate .yml layer. + existing = _find_overlay_file(project_root, overlay.extends, overlay.id) + if existing is not None: + target_path = existing + else: + target_path = _ensure_contained_path( + target_dir / f"{overlay.id}.yml", _overlay_root(project_root) + ) try: target_path.write_text(yaml.safe_dump(data, sort_keys=False), encoding="utf-8") diff --git a/src/specify_cli/workflows/overlays/schema.py b/src/specify_cli/workflows/overlays/schema.py index 5652d3f713..dc25c4a9ad 100644 --- a/src/specify_cli/workflows/overlays/schema.py +++ b/src/specify_cli/workflows/overlays/schema.py @@ -41,7 +41,7 @@ def _validate_safe_id(value: str, field_name: str, allow_reserved: bool = False) """Return an error message if *value* is not a safe path segment ID.""" if not isinstance(value, str) or not value: return f"Overlay '{field_name}' is required and must be a non-empty string." - if not _SAFE_ID_PATTERN.match(value): + if not _SAFE_ID_PATTERN.fullmatch(value): return ( f"Overlay '{field_name}' {value!r} contains invalid characters; " "only lowercase letters, digits, and hyphens are allowed." diff --git a/tests/workflows/test_overlay_commands.py b/tests/workflows/test_overlay_commands.py index 5e606f3a53..c7d8ccf011 100644 --- a/tests/workflows/test_overlay_commands.py +++ b/tests/workflows/test_overlay_commands.py @@ -82,6 +82,58 @@ def test_overlay_add(self, project_dir, monkeypatch): data = yaml.safe_load(installed.read_text(encoding="utf-8")) assert data["priority"] == 5 + def test_overlay_add_reuses_yaml_extension(self, project_dir, monkeypatch): + """If .yaml already exists, overlay add must write to it instead of creating .yml.""" + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + _write_workflow( + project_dir, + "wf", + { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "echo"}], + }, + ) + # Pre-create the overlay using the .yaml extension. + existing_yaml = project_dir / ".specify" / "workflows" / "overlays" / "wf" / "ov1.yaml" + existing_yaml.parent.mkdir(parents=True, exist_ok=True) + existing_yaml.write_text( + yaml.safe_dump( + { + "id": "ov1", + "extends": "wf", + "priority": 1, + "edits": [{"remove": "a"}], + } + ), + encoding="utf-8", + ) + + overlay_file = project_dir / "overlay.yml" + overlay_file.write_text( + yaml.safe_dump( + { + "id": "ov1", + "extends": "wf", + "priority": 20, + "edits": [{"remove": "a"}], + } + ), + encoding="utf-8", + ) + + result = runner.invoke(app, ["workflow", "overlay", "add", str(overlay_file)]) + assert result.exit_code == 0, result.output + + # Should have written to the pre-existing .yaml file. + assert existing_yaml.is_file() + data = yaml.safe_load(existing_yaml.read_text(encoding="utf-8")) + assert data["priority"] == 20 + + # Must NOT have created a duplicate .yml alongside the .yaml. + duplicate_yml = existing_yaml.with_suffix(".yml") + assert not duplicate_yml.exists(), "duplicate .yml was created alongside existing .yaml" + def test_overlay_add_with_priority_override_missing_in_file(self, project_dir, monkeypatch): """--priority must fix a missing priority in the overlay file.""" monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) diff --git a/tests/workflows/test_overlay_schema.py b/tests/workflows/test_overlay_schema.py index 72595b6d22..026797b0d8 100644 --- a/tests/workflows/test_overlay_schema.py +++ b/tests/workflows/test_overlay_schema.py @@ -216,3 +216,29 @@ def test_valid_dashed_id_accepted(self): ) assert not errors, errors assert overlay is not None + + def test_validate_safe_id_rejects_trailing_newline(self): + """A trailing newline must not pass ID validation (fullmatch guard).""" + overlay, errors = validate_overlay_yaml( + { + "id": "overlay\n", + "extends": "wf", + "priority": 10, + "edits": [{"remove": "a"}], + } + ) + assert overlay is None + assert any("id" in e.lower() for e in errors), errors + + def test_validate_safe_id_rejects_embedded_newline(self): + """An embedded newline must not pass ID validation.""" + overlay, errors = validate_overlay_yaml( + { + "id": "ov\nerlay", + "extends": "wf", + "priority": 10, + "edits": [{"remove": "a"}], + } + ) + assert overlay is None + assert any("id" in e.lower() for e in errors), errors From c1e0683d965094f92474733044634f44f925f5dc Mon Sep 17 00:00:00 2001 From: Markus Date: Fri, 17 Jul 2026 17:04:29 +0200 Subject: [PATCH 10/29] fix(overlays): fix display order inversion and wrap file-read errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finding group 3 from copilot-review-v2.md: 3.1 — Precedence display inverted (overlays/__init__.py) collect_all_layers used a single-pass sort by (-priority, source_asc), which placed the *losing* equal-priority source first in the display while claiming "highest first". Fix: two-pass stable sort — source descending then priority descending — so the actual winner (last applied by the composer) rises to the top of the display. 3.2 — Unwrapped file-read errors (overlays/layer_sources.py) Only yaml.YAMLError was caught around path.read_text(), so an unreadable or non-UTF-8 overlay produced a raw traceback. Fix: widen the except clause to (yaml.YAMLError, OSError, UnicodeDecodeError), matching the pattern used throughout catalog.py. Tests: - test_workflow_resolve_equal_priority_winner_shown_first: verifies project:zzz (the winner) appears before project:aaa in workflow resolve output when both overlays share the same priority. - tests/workflows/test_overlay_layer_sources.py (new): OSError and non-UTF-8 bytes both produce OverlayLoadError, not raw tracebacks. Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../workflows/overlays/__init__.py | 14 +++-- .../workflows/overlays/layer_sources.py | 4 +- tests/workflows/test_overlay_commands.py | 58 ++++++++++++++++++ tests/workflows/test_overlay_layer_sources.py | 60 +++++++++++++++++++ 4 files changed, 129 insertions(+), 7 deletions(-) create mode 100644 tests/workflows/test_overlay_layer_sources.py diff --git a/src/specify_cli/workflows/overlays/__init__.py b/src/specify_cli/workflows/overlays/__init__.py index 945ba40d02..779b6a57fd 100644 --- a/src/specify_cli/workflows/overlays/__init__.py +++ b/src/specify_cli/workflows/overlays/__init__.py @@ -36,16 +36,20 @@ def __init__(self, project_root: Path) -> None: def collect_all_layers(self, workflow_id: str) -> list[Layer]: """Collect all layers sorted by resolver precedence. - Higher priority wins. Ties are broken alphabetically by source. + Higher priority wins. Ties are broken so the actual winner appears first: + the composer applies equal-priority sources ascending (last wins), so we + reverse source ordering here to show the winner at the top of the list. + Two stable passes: source descending, then priority descending. """ all_layers: list[Layer] = [] for source in self._sources: all_layers.extend(source.collect(workflow_id)) - return sorted( - all_layers, - key=lambda layer: (-layer.priority, layer.source), - ) + # Pass 1: source descending — within each priority group the winning + # source (alphabetically last = applied last by the composer) rises first. + by_source = sorted(all_layers, key=lambda layer: layer.source, reverse=True) + # Pass 2: priority descending — overall ordering by precedence. + return sorted(by_source, key=lambda layer: layer.priority, reverse=True) def resolve(self, workflow_id: str) -> WorkflowDefinition: """Resolve a workflow ID to its composed definition. diff --git a/src/specify_cli/workflows/overlays/layer_sources.py b/src/specify_cli/workflows/overlays/layer_sources.py index 9ce64177ca..88318b3ad4 100644 --- a/src/specify_cli/workflows/overlays/layer_sources.py +++ b/src/specify_cli/workflows/overlays/layer_sources.py @@ -98,8 +98,8 @@ def collect(self, workflow_id: str) -> list[Layer]: raise OverlayLoadError(path, ["Symlinked overlay files are not allowed"]) try: data = yaml.safe_load(path.read_text(encoding="utf-8")) or {} - except yaml.YAMLError as exc: - raise OverlayLoadError(path, [f"Invalid YAML: {exc}"]) from exc + except (yaml.YAMLError, OSError, UnicodeDecodeError) as exc: + raise OverlayLoadError(path, [f"Cannot load overlay: {exc}"]) from exc overlay, errors = validate_overlay_yaml(data) if overlay is None or errors: raise OverlayLoadError(path, errors) diff --git a/tests/workflows/test_overlay_commands.py b/tests/workflows/test_overlay_commands.py index c7d8ccf011..3da58baa57 100644 --- a/tests/workflows/test_overlay_commands.py +++ b/tests/workflows/test_overlay_commands.py @@ -366,6 +366,64 @@ def test_workflow_resolve(self, project_dir, monkeypatch): assert "project:ov1" in result.output assert "new" in result.output + def test_workflow_resolve_equal_priority_winner_shown_first(self, project_dir, monkeypatch): + """Equal-priority overlays must be displayed with the actual winner (last applied) first.""" + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + _write_workflow( + project_dir, + "wf", + { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "echo"}], + }, + ) + # "zzz" sorts last alphabetically, so the composer applies it last → it wins. + # "aaa" sorts first, so it is applied first → it loses. + # The display must show the winner ("project:zzz") before the loser ("project:aaa"). + _write_overlay( + project_dir, + "wf", + "aaa", + { + "id": "aaa", + "extends": "wf", + "priority": 10, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "aaa-step", "type": "command", "command": "echo"}, + } + ], + }, + ) + _write_overlay( + project_dir, + "wf", + "zzz", + { + "id": "zzz", + "extends": "wf", + "priority": 10, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "zzz-step", "type": "command", "command": "echo"}, + } + ], + }, + ) + + result = runner.invoke(app, ["workflow", "resolve", "wf"]) + assert result.exit_code == 0, result.output + zzz_pos = result.output.index("project:zzz") + aaa_pos = result.output.index("project:aaa") + assert zzz_pos < aaa_pos, ( + "project:zzz (the winning source) should appear before project:aaa in the output" + ) + def test_workflow_add_copies_overlays(self, project_dir, monkeypatch, tmp_path): monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) source_dir = tmp_path / "source-wf" diff --git a/tests/workflows/test_overlay_layer_sources.py b/tests/workflows/test_overlay_layer_sources.py new file mode 100644 index 0000000000..befebb4648 --- /dev/null +++ b/tests/workflows/test_overlay_layer_sources.py @@ -0,0 +1,60 @@ +"""Tests for ProjectOverlaySource file-read error handling.""" + +from __future__ import annotations + +from pathlib import Path +from unittest.mock import patch + +import pytest +import yaml + +from specify_cli.workflows.overlays.layer_sources import ( + OverlayLoadError, + ProjectOverlaySource, +) + + +@pytest.fixture +def project_dir(tmp_path: Path) -> Path: + workflows_dir = tmp_path / ".specify" / "workflows" + workflows_dir.mkdir(parents=True, exist_ok=True) + return tmp_path + + +def _write_overlay_file(project_dir: Path, workflow_id: str, overlay_id: str, data: dict) -> Path: + ov_dir = project_dir / ".specify" / "workflows" / "overlays" / workflow_id + ov_dir.mkdir(parents=True, exist_ok=True) + path = ov_dir / f"{overlay_id}.yml" + path.write_text(yaml.safe_dump(data), encoding="utf-8") + return path + + +class TestProjectOverlaySourceFileReadErrors: + """File-read errors must be wrapped in OverlayLoadError, not leaked as raw tracebacks.""" + + def test_oserror_raises_overlay_load_error(self, project_dir: Path) -> None: + """An OSError from read_text (e.g. permission denied) is wrapped in OverlayLoadError.""" + _write_overlay_file( + project_dir, + "wf", + "ov1", + {"id": "ov1", "extends": "wf", "priority": 5, "edits": []}, + ) + source = ProjectOverlaySource(project_dir) + with patch.object(Path, "read_text", side_effect=OSError("Permission denied")): + with pytest.raises(OverlayLoadError) as exc_info: + source.collect("wf") + assert exc_info.value.errors, "OverlayLoadError must carry a non-empty errors list" + + def test_unicode_error_raises_overlay_load_error(self, project_dir: Path) -> None: + """A file containing non-UTF-8 bytes raises OverlayLoadError, not UnicodeDecodeError.""" + ov_dir = project_dir / ".specify" / "workflows" / "overlays" / "wf" + ov_dir.mkdir(parents=True, exist_ok=True) + # Write raw invalid UTF-8 bytes directly so read_text(encoding="utf-8") fails. + bad_file = ov_dir / "bad.yml" + bad_file.write_bytes(b"\xff\xfe invalid utf-8") + + source = ProjectOverlaySource(project_dir) + with pytest.raises(OverlayLoadError) as exc_info: + source.collect("wf") + assert exc_info.value.errors, "OverlayLoadError must carry a non-empty errors list" From 2d5b14384cc78bbe3c1cb4f9de1ac5a137c48748 Mon Sep 17 00:00:00 2001 From: Markus Date: Fri, 17 Jul 2026 17:07:00 +0200 Subject: [PATCH 11/29] fix: rename misleading overlay test Assisted-by: GitHub Copilot (model: gpt-5.3-codex, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/workflows/test_overlay_commands.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/workflows/test_overlay_commands.py b/tests/workflows/test_overlay_commands.py index 3da58baa57..f8dc0ba072 100644 --- a/tests/workflows/test_overlay_commands.py +++ b/tests/workflows/test_overlay_commands.py @@ -424,7 +424,7 @@ def test_workflow_resolve_equal_priority_winner_shown_first(self, project_dir, m "project:zzz (the winning source) should appear before project:aaa in the output" ) - def test_workflow_add_copies_overlays(self, project_dir, monkeypatch, tmp_path): + def test_workflow_add_does_not_copy_overlays(self, project_dir, monkeypatch, tmp_path): monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) source_dir = tmp_path / "source-wf" source_dir.mkdir() From 0a6dc1361badf04bb94e94d3bea8507bb6875685 Mon Sep 17 00:00:00 2001 From: Markus Date: Fri, 17 Jul 2026 17:10:09 +0200 Subject: [PATCH 12/29] fix: remove EOF blank line in overlay resolver Assisted-by: GitHub Copilot (model: gpt-5.3-codex, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/workflows/overlays/__init__.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/specify_cli/workflows/overlays/__init__.py b/src/specify_cli/workflows/overlays/__init__.py index 779b6a57fd..40521a8feb 100644 --- a/src/specify_cli/workflows/overlays/__init__.py +++ b/src/specify_cli/workflows/overlays/__init__.py @@ -73,4 +73,3 @@ def resolve_with_layers( if definition is None: raise FileNotFoundError(f"Workflow not found: {workflow_id}") return definition, layers, attribution - From b4bb6f88a64aaeb323336eb242157ba85ad91a5d Mon Sep 17 00:00:00 2001 From: Markus Wondrak Date: Fri, 17 Jul 2026 18:42:34 +0200 Subject: [PATCH 13/29] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/specify_cli/workflows/overlays/layer_sources.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/workflows/overlays/layer_sources.py b/src/specify_cli/workflows/overlays/layer_sources.py index 88318b3ad4..0b631a6db9 100644 --- a/src/specify_cli/workflows/overlays/layer_sources.py +++ b/src/specify_cli/workflows/overlays/layer_sources.py @@ -98,7 +98,9 @@ def collect(self, workflow_id: str) -> list[Layer]: raise OverlayLoadError(path, ["Symlinked overlay files are not allowed"]) try: data = yaml.safe_load(path.read_text(encoding="utf-8")) or {} - except (yaml.YAMLError, OSError, UnicodeDecodeError) as exc: + except yaml.YAMLError as exc: + raise OverlayLoadError(path, [f"Invalid YAML: {exc}"]) from exc + except (OSError, UnicodeDecodeError) as exc: raise OverlayLoadError(path, [f"Cannot load overlay: {exc}"]) from exc overlay, errors = validate_overlay_yaml(data) if overlay is None or errors: From 21dda03415451a99183cb2e41c42ca320f10cfbb Mon Sep 17 00:00:00 2001 From: Markus Date: Fri, 17 Jul 2026 18:44:24 +0200 Subject: [PATCH 14/29] fix: handle overlay read and enumeration errors Assisted-by: GitHub Copilot (model: gpt-5.3-codex, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/workflows/overlays/_commands.py | 2 +- src/specify_cli/workflows/overlays/layer_sources.py | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/workflows/overlays/_commands.py b/src/specify_cli/workflows/overlays/_commands.py index c17450261a..dcba31160e 100644 --- a/src/specify_cli/workflows/overlays/_commands.py +++ b/src/specify_cli/workflows/overlays/_commands.py @@ -137,7 +137,7 @@ def _read_overlay(path: Path) -> tuple[dict[str, Any] | None, list[str]]: """Read and parse an overlay YAML file, returning (data, errors).""" try: content = path.read_text(encoding="utf-8") - except OSError as exc: + except (OSError, UnicodeDecodeError) as exc: return None, [f"Failed to read {path}: {exc}"] try: data = yaml.safe_load(content) diff --git a/src/specify_cli/workflows/overlays/layer_sources.py b/src/specify_cli/workflows/overlays/layer_sources.py index 0b631a6db9..4c29ec97f3 100644 --- a/src/specify_cli/workflows/overlays/layer_sources.py +++ b/src/specify_cli/workflows/overlays/layer_sources.py @@ -91,7 +91,13 @@ def collect(self, workflow_id: str) -> list[Layer]: if not workflow_overlay_dir.is_dir(): return [] layers: list[Layer] = [] - for path in sorted(workflow_overlay_dir.iterdir()): + try: + entries = sorted(workflow_overlay_dir.iterdir()) + except OSError as exc: + raise OverlayLoadError( + workflow_overlay_dir, [f"Cannot enumerate overlays: {exc}"] + ) from exc + for path in entries: if not path.is_file() or path.suffix not in (".yml", ".yaml"): continue if path.is_symlink(): From 0969d9f8b48d701cfafa32c60306696fa5a45187 Mon Sep 17 00:00:00 2001 From: Markus Date: Fri, 17 Jul 2026 19:15:52 +0200 Subject: [PATCH 15/29] fix(overlays): validate resolver workflow IDs Reject unsafe and reserved workflow IDs before overlay or base sources construct paths, preventing traversal through resolver and engine fallback paths. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../workflows/overlays/__init__.py | 13 ++++ .../workflows/overlays/_commands.py | 4 +- src/specify_cli/workflows/overlays/schema.py | 16 ++++- tests/workflows/test_overlay_schema.py | 13 ++++ tests/workflows/test_overlay_security.py | 7 +- tests/workflows/test_resolver_integration.py | 72 +++++++++++++++++++ 6 files changed, 118 insertions(+), 7 deletions(-) diff --git a/src/specify_cli/workflows/overlays/__init__.py b/src/specify_cli/workflows/overlays/__init__.py index 40521a8feb..017645560f 100644 --- a/src/specify_cli/workflows/overlays/__init__.py +++ b/src/specify_cli/workflows/overlays/__init__.py @@ -12,6 +12,17 @@ ProjectOverlaySource, ) from .merge import ComposedStep +from .schema import _RESERVED_WORKFLOW_IDS, _SAFE_ID_PATTERN + + +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 workflow_id in _RESERVED_WORKFLOW_IDS + ): + raise ValueError(f"Invalid workflow ID: {workflow_id!r}") class WorkflowResolver: @@ -41,6 +52,8 @@ def collect_all_layers(self, workflow_id: str) -> list[Layer]: reverse source ordering here to show the winner at the top of the list. Two stable passes: source descending, then priority descending. """ + _validate_workflow_id(workflow_id) + all_layers: list[Layer] = [] for source in self._sources: all_layers.extend(source.collect(workflow_id)) diff --git a/src/specify_cli/workflows/overlays/_commands.py b/src/specify_cli/workflows/overlays/_commands.py index dcba31160e..3dc6a16a14 100644 --- a/src/specify_cli/workflows/overlays/_commands.py +++ b/src/specify_cli/workflows/overlays/_commands.py @@ -11,7 +11,7 @@ from ..._console import console, err_console from .._commands import _reject_unsafe_dir, _reject_unsafe_workflow_storage from . import WorkflowResolver -from .schema import _RESERVED_OVERLAY_WORKFLOW_IDS, _SAFE_ID_PATTERN, validate_overlay_yaml +from .schema import _RESERVED_WORKFLOW_IDS, _SAFE_ID_PATTERN, validate_overlay_yaml def _validate_overlay_id_or_exit(id_value: str, label: str) -> None: @@ -30,7 +30,7 @@ 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 workflow_id in _RESERVED_OVERLAY_WORKFLOW_IDS: + if workflow_id in _RESERVED_WORKFLOW_IDS: err_console.print( f"[red]Error:[/red] Invalid workflow ID {workflow_id!r}: " "reserved name." diff --git a/src/specify_cli/workflows/overlays/schema.py b/src/specify_cli/workflows/overlays/schema.py index dc25c4a9ad..52a907c0c3 100644 --- a/src/specify_cli/workflows/overlays/schema.py +++ b/src/specify_cli/workflows/overlays/schema.py @@ -10,6 +10,7 @@ # 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"}) +_RESERVED_WORKFLOW_IDS: frozenset[str] = frozenset({"overlays", "runs", "steps"}) VALID_OPERATIONS = frozenset({"insert_after", "insert_before", "replace", "remove"}) @@ -37,7 +38,12 @@ class Overlay: enabled: bool = True -def _validate_safe_id(value: str, field_name: str, allow_reserved: bool = False) -> str | None: +def _validate_safe_id( + value: str, + field_name: str, + allow_reserved: bool = False, + reserved_ids: frozenset[str] = _RESERVED_OVERLAY_WORKFLOW_IDS, +) -> str | None: """Return an error message if *value* is not a safe path segment ID.""" if not isinstance(value, str) or not value: return f"Overlay '{field_name}' is required and must be a non-empty string." @@ -46,7 +52,7 @@ def _validate_safe_id(value: str, field_name: str, allow_reserved: bool = False) f"Overlay '{field_name}' {value!r} contains invalid characters; " "only lowercase letters, digits, and hyphens are allowed." ) - if not allow_reserved and value in _RESERVED_OVERLAY_WORKFLOW_IDS: + if not allow_reserved and value in reserved_ids: return f"Overlay '{field_name}' {value!r} is reserved." return None @@ -121,7 +127,11 @@ def validate_overlay_yaml(data: dict[str, Any]) -> tuple[Overlay | None, list[st overlay_id = "" extends = data.get("extends") - if err := _validate_safe_id(extends, "extends"): + if err := _validate_safe_id( + extends, + "extends", + reserved_ids=_RESERVED_WORKFLOW_IDS, + ): errors.append(err) extends = "" diff --git a/tests/workflows/test_overlay_schema.py b/tests/workflows/test_overlay_schema.py index 026797b0d8..96bd541df3 100644 --- a/tests/workflows/test_overlay_schema.py +++ b/tests/workflows/test_overlay_schema.py @@ -205,6 +205,19 @@ def test_invalid_extends_rejected(self, extends): assert overlay is None assert any("extends" in e.lower() for e in errors), errors + @pytest.mark.parametrize("extends", ["overlays", "runs", "steps"]) + def test_reserved_workflow_id_rejected(self, extends): + overlay, errors = validate_overlay_yaml( + { + "id": "ov", + "extends": extends, + "priority": 10, + "edits": [{"remove": "a"}], + } + ) + assert overlay is None + assert any("reserved" in error.lower() for error in errors), errors + def test_valid_dashed_id_accepted(self): overlay, errors = validate_overlay_yaml( { diff --git a/tests/workflows/test_overlay_security.py b/tests/workflows/test_overlay_security.py index 505b438d2b..a1249ad841 100644 --- a/tests/workflows/test_overlay_security.py +++ b/tests/workflows/test_overlay_security.py @@ -209,9 +209,12 @@ def test_overlay_add_rejects_symlinked_target_file(self, project_dir, monkeypatc assert "symlinked path" in result.output.lower() assert real_file.read_text(encoding="utf-8") == "sentinel\n" - def test_overlay_operations_reject_overlays_as_workflow_id(self, project_dir, monkeypatch): + @pytest.mark.parametrize("workflow_id", ["overlays", "runs", "steps"]) + def test_overlay_operations_reject_reserved_workflow_id( + self, project_dir, monkeypatch, workflow_id + ): monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) - result = runner.invoke(app, ["workflow", "overlay", "list", "overlays"]) + result = runner.invoke(app, ["workflow", "overlay", "list", workflow_id]) assert result.exit_code != 0, result.output assert "Invalid" in result.output or "reserved" in result.output.lower() diff --git a/tests/workflows/test_resolver_integration.py b/tests/workflows/test_resolver_integration.py index afd3461bd4..a007fdb55a 100644 --- a/tests/workflows/test_resolver_integration.py +++ b/tests/workflows/test_resolver_integration.py @@ -31,6 +31,46 @@ def _write_overlay(project_root: Path, workflow_id: str, overlay_id: str, data: class TestWorkflowResolver: """End-to-end resolution of base workflows plus overlays.""" + @pytest.mark.parametrize( + "workflow_id", + [ + "../outside", + "nested/workflow", + "wf\n", + "overlays", + "runs", + "steps", + ], + ) + def test_rejects_unsafe_id_before_collecting_sources( + self, project_dir, workflow_id + ): + resolver = WorkflowResolver(project_dir) + + class UnexpectedSource: + def collect(self, _workflow_id): + pytest.fail("source collection must not run for an unsafe workflow ID") + + resolver._sources = [UnexpectedSource()] + + with pytest.raises(ValueError, match="Invalid workflow ID"): + resolver.resolve(workflow_id) + + def test_rejects_absolute_id_before_collecting_sources( + self, project_dir, tmp_path + ): + resolver = WorkflowResolver(project_dir) + outside = tmp_path / "outside" + + class UnexpectedSource: + def collect(self, _workflow_id): + pytest.fail("source collection must not run for an absolute workflow ID") + + resolver._sources = [UnexpectedSource()] + + with pytest.raises(ValueError, match="Invalid workflow ID"): + resolver.resolve(str(outside)) + def test_resolve_without_overlays(self, project_dir): data = { "schema_version": "1.0", @@ -459,3 +499,35 @@ def test_engine_load_workflow_uses_resolver(self, project_dir): engine = WorkflowEngine(project_dir) definition = engine.load_workflow("wf") assert [s["id"] for s in definition.steps] == ["a", "new"] + + def test_engine_rejects_traversal_without_legacy_path_fallback( + self, project_dir + ): + from specify_cli.workflows.engine import WorkflowEngine + + outside = project_dir / ".specify" / "outside" + outside.mkdir(parents=True) + (outside / "workflow.yml").write_text( + yaml.safe_dump( + { + "schema_version": "1.0", + "workflow": { + "id": "outside", + "name": "Outside", + "version": "1.0.0", + }, + "steps": [ + { + "id": "external", + "type": "command", + "command": "echo", + } + ], + } + ), + encoding="utf-8", + ) + + engine = WorkflowEngine(project_dir) + with pytest.raises(ValueError, match="Invalid workflow ID"): + engine.load_workflow("../outside") From 010fb670757a4d79f4bc3894533eddf91e9de588 Mon Sep 17 00:00:00 2001 From: Markus Wondrak Date: Fri, 17 Jul 2026 19:25:54 +0200 Subject: [PATCH 16/29] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/specify_cli/workflows/overlays/_commands.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/specify_cli/workflows/overlays/_commands.py b/src/specify_cli/workflows/overlays/_commands.py index c17450261a..dcba31160e 100644 --- a/src/specify_cli/workflows/overlays/_commands.py +++ b/src/specify_cli/workflows/overlays/_commands.py @@ -137,7 +137,7 @@ def _read_overlay(path: Path) -> tuple[dict[str, Any] | None, list[str]]: """Read and parse an overlay YAML file, returning (data, errors).""" try: content = path.read_text(encoding="utf-8") - except OSError as exc: + except (OSError, UnicodeDecodeError) as exc: return None, [f"Failed to read {path}: {exc}"] try: data = yaml.safe_load(content) From 28c85fff368040474debbf9008181de7b80612e5 Mon Sep 17 00:00:00 2001 From: Markus Date: Fri, 17 Jul 2026 19:26:30 +0200 Subject: [PATCH 17/29] fix(overlays): drop _remove_sources_recursively from remove branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In _traverse_and_apply, the remove branch called _remove_sources_recursively to clean up attribution entries for the deleted step. This was inherited from the old apply_edit loop (c70a5d6) where it was needed because the sources dict was queried exhaustively. In the current single-pass design, _build_attribution only traverses the result list, so stale sources entries for removed steps are never read. The cleanup call is therefore unnecessary — and actively harmful when another overlay has replaced a different step with a new step that reuses the same ID: the pop clobbers the replacement's attribution entry, causing workflow resolve to report the surviving step as 'unknown'. Fix: simply remove the _remove_sources_recursively call from the remove branch. Add an attribution assertion to the existing reused-ID regression test to catch this case. Fixes: r3604242050 (Copilot review finding) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) --- src/specify_cli/workflows/overlays/merge.py | 6 +++++- tests/workflows/test_overlay_merge.py | 10 +++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/workflows/overlays/merge.py b/src/specify_cli/workflows/overlays/merge.py index a1a6fdd607..a734bf8c80 100644 --- a/src/specify_cli/workflows/overlays/merge.py +++ b/src/specify_cli/workflows/overlays/merge.py @@ -259,7 +259,11 @@ def _traverse_and_apply( if winning_edit is not None and winning_edit.operation == "remove": # Winning edit removes this step; ignore all other edits on this anchor. - _remove_sources_recursively(step, sources) + # Do NOT call _remove_sources_recursively here: _build_attribution only + # traverses the result list, so stale sources entries for removed steps + # are never read. Calling it would incorrectly pop the attribution of a + # *surviving* step that reuses the same ID (e.g. a replacement step + # introduced by a higher-priority overlay targeting a different anchor). continue # Insert before (in merge order). diff --git a/tests/workflows/test_overlay_merge.py b/tests/workflows/test_overlay_merge.py index b699b92c31..17790d6801 100644 --- a/tests/workflows/test_overlay_merge.py +++ b/tests/workflows/test_overlay_merge.py @@ -700,7 +700,7 @@ def test_replace_with_reused_id_does_not_affect_original(self): OverlayEdit("remove", "b"), ], ) - steps, _ = merge_steps(base, [_layer(overlay, "project:ov")]) + steps, attribution = merge_steps(base, [_layer(overlay, "project:ov")]) # The original "b" is removed; the replacement (also id="b") survives. # "c" is untouched. assert len(steps) == 2 @@ -708,3 +708,11 @@ def test_replace_with_reused_id_does_not_affect_original(self): assert remaining_ids == ["b", "c"] # The surviving "b" step is the replacement (has the custom command). assert steps[0]["command"] == "speckit.replaced" + # Attribution for the surviving replacement "b" must not be "unknown". + # Previously, removing original "b" would pop sources["b"], erasing the + # attribution recorded for the replacement step (regression guard for the + # _remove_sources_recursively-in-remove-branch bug). + sources = {cs.step_id: cs.source for cs in attribution} + assert sources.get("b") == "project:ov", ( + f"expected 'project:ov' but got {sources.get('b')!r}" + ) From 9e6aa657b80d936335aca336ffbee60d1b394fbd Mon Sep 17 00:00:00 2001 From: Markus Date: Fri, 17 Jul 2026 19:28:34 +0200 Subject: [PATCH 18/29] Fix CLI overlay ID validation anchoring Use fullmatch for CLI workflow/overlay ID validation so trailing newlines are rejected consistently with manifest validation. Add regression coverage for newline-suffixed workflow and overlay IDs in overlay set-priority. Assisted-by: GitHub Copilot (model: gpt-5.3-codex, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../workflows/overlays/_commands.py | 2 +- tests/workflows/test_overlay_commands.py | 41 +++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/workflows/overlays/_commands.py b/src/specify_cli/workflows/overlays/_commands.py index 3dc6a16a14..4febb73d66 100644 --- a/src/specify_cli/workflows/overlays/_commands.py +++ b/src/specify_cli/workflows/overlays/_commands.py @@ -19,7 +19,7 @@ def _validate_overlay_id_or_exit(id_value: str, label: str) -> None: if not isinstance(id_value, str) or not id_value: err_console.print(f"[red]Error:[/red] {label} is required and must be a non-empty string.") raise typer.Exit(1) - if not _SAFE_ID_PATTERN.match(id_value): + if not _SAFE_ID_PATTERN.fullmatch(id_value): err_console.print( f"[red]Error:[/red] Invalid {label} {id_value!r}: " "only lowercase letters, digits, and hyphens are allowed." diff --git a/tests/workflows/test_overlay_commands.py b/tests/workflows/test_overlay_commands.py index f8dc0ba072..c5a11aa1f3 100644 --- a/tests/workflows/test_overlay_commands.py +++ b/tests/workflows/test_overlay_commands.py @@ -216,6 +216,47 @@ def test_overlay_set_priority(self, project_dir, monkeypatch): ) assert data["priority"] == 20 + def test_overlay_set_priority_rejects_ids_with_trailing_newline(self, project_dir, monkeypatch): + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + _write_workflow( + project_dir, + "wf", + { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "echo"}], + }, + ) + _write_overlay( + project_dir, + "wf", + "ov1", + { + "id": "ov1", + "extends": "wf", + "priority": 10, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "new", "type": "command", "command": "echo"}, + } + ], + }, + ) + + result = runner.invoke( + app, ["workflow", "overlay", "set-priority", "wf", "ov1\n", "20"] + ) + assert result.exit_code == 1 + assert "Invalid overlay ID" in result.output + + result = runner.invoke( + app, ["workflow", "overlay", "set-priority", "wf\n", "ov1", "20"] + ) + assert result.exit_code == 1 + assert "Invalid workflow ID" in result.output + def test_overlay_disable_and_enable(self, project_dir, monkeypatch): monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) _write_workflow( From b9de204a9e2b222da28014366acc6a7a5661dc5c Mon Sep 17 00:00:00 2001 From: Markus Date: Fri, 17 Jul 2026 19:33:38 +0200 Subject: [PATCH 19/29] fix(workflows): validate workflow_id in layer sources before path construction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ProjectOverlaySource.collect() and BaseWorkflowSource.collect() joined workflow_id directly onto storage paths without validation, enabling path traversal (e.g. '../../outside') when called outside the WorkflowResolver. Add _validate_workflow_id() to layer_sources.py — mirrors the same _SAFE_ID_PATTERN / _RESERVED_WORKFLOW_IDS check used by WorkflowResolver in overlays/__init__.py — and call it at the top of both collect() methods before any path is constructed. Adds parametrised tests covering unsafe IDs and verifying no filesystem access occurs for an invalid ID. Closes review finding r3604772700. Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../workflows/overlays/layer_sources.py | 21 +++++++- tests/workflows/test_overlay_layer_sources.py | 49 ++++++++++++++++++- 2 files changed, 68 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/workflows/overlays/layer_sources.py b/src/specify_cli/workflows/overlays/layer_sources.py index 4c29ec97f3..5ced4713ab 100644 --- a/src/specify_cli/workflows/overlays/layer_sources.py +++ b/src/specify_cli/workflows/overlays/layer_sources.py @@ -7,7 +7,7 @@ import yaml -from .schema import Overlay, validate_overlay_yaml +from .schema import Overlay, _RESERVED_WORKFLOW_IDS, _SAFE_ID_PATTERN, validate_overlay_yaml @dataclass @@ -30,6 +30,23 @@ def __init__(self, path: Path, errors: list[str]) -> None: super().__init__(f"Invalid overlay {path}:\n - " + "\n - ".join(errors)) +def _validate_workflow_id(workflow_id: str, context_path: Path) -> None: + """Raise OverlayLoadError if workflow_id is not a safe path-segment identifier. + + Mirrors the same check performed by WorkflowResolver so layer sources are + safe to call directly, without going through the resolver. + """ + if ( + not isinstance(workflow_id, str) + or not _SAFE_ID_PATTERN.fullmatch(workflow_id) + or workflow_id in _RESERVED_WORKFLOW_IDS + ): + raise OverlayLoadError( + context_path, + [f"Invalid workflow ID: {workflow_id!r}"], + ) + + def _resolve_project_overlay_root(project_root: Path) -> Path: """Return the unresolved overlay root after rejecting symlinked ancestors. @@ -77,6 +94,7 @@ def __init__(self, project_root: Path) -> None: def collect(self, workflow_id: str) -> list[Layer]: """Collect all project-local overlays for the given workflow id.""" self.overlays_dir = _resolve_project_overlay_root(self.project_root) + _validate_workflow_id(workflow_id, self.overlays_dir) workflow_overlay_dir = self.overlays_dir / workflow_id if workflow_overlay_dir.is_symlink(): raise OverlayLoadError( @@ -138,6 +156,7 @@ def __init__(self, project_root: Path) -> None: def collect(self, workflow_id: str) -> list[Layer]: """Return the base workflow as a single layer if it exists.""" + _validate_workflow_id(workflow_id, self.workflows_dir) path = self.workflows_dir / workflow_id / "workflow.yml" if not path.is_file(): return [] diff --git a/tests/workflows/test_overlay_layer_sources.py b/tests/workflows/test_overlay_layer_sources.py index befebb4648..9db33807ba 100644 --- a/tests/workflows/test_overlay_layer_sources.py +++ b/tests/workflows/test_overlay_layer_sources.py @@ -1,4 +1,4 @@ -"""Tests for ProjectOverlaySource file-read error handling.""" +"""Tests for ProjectOverlaySource and BaseWorkflowSource.""" from __future__ import annotations @@ -9,6 +9,7 @@ import yaml from specify_cli.workflows.overlays.layer_sources import ( + BaseWorkflowSource, OverlayLoadError, ProjectOverlaySource, ) @@ -58,3 +59,49 @@ def test_unicode_error_raises_overlay_load_error(self, project_dir: Path) -> Non with pytest.raises(OverlayLoadError) as exc_info: source.collect("wf") assert exc_info.value.errors, "OverlayLoadError must carry a non-empty errors list" + + +_UNSAFE_IDS = [ + "../outside", + "../../escape", + "nested/workflow", + "wf\n", + "overlays", + "runs", + "steps", + "", + "/absolute", + "UPPER", + "has space", +] + + +class TestProjectOverlaySourceIdValidation: + """ProjectOverlaySource.collect() must reject unsafe IDs before path construction.""" + + @pytest.mark.parametrize("workflow_id", _UNSAFE_IDS) + def test_rejects_unsafe_id(self, project_dir: Path, workflow_id: str) -> None: + source = ProjectOverlaySource(project_dir) + with pytest.raises(OverlayLoadError, match="Invalid workflow ID"): + source.collect(workflow_id) + + @pytest.mark.parametrize("workflow_id", _UNSAFE_IDS) + def test_does_not_access_filesystem_for_unsafe_id( + self, project_dir: Path, workflow_id: str + ) -> None: + """No directory walk or file read should happen for an invalid ID.""" + source = ProjectOverlaySource(project_dir) + with patch.object(Path, "iterdir", side_effect=AssertionError("iterdir called")): + with pytest.raises(OverlayLoadError, match="Invalid workflow ID"): + source.collect(workflow_id) + + +class TestBaseWorkflowSourceIdValidation: + """BaseWorkflowSource.collect() must reject unsafe IDs before path construction.""" + + @pytest.mark.parametrize("workflow_id", _UNSAFE_IDS) + def test_rejects_unsafe_id(self, project_dir: Path, workflow_id: str) -> None: + source = BaseWorkflowSource(project_dir) + with pytest.raises(OverlayLoadError, match="Invalid workflow ID"): + source.collect(workflow_id) + From 69301a679a97deef3d0f6ef545a5d211ea64dd45 Mon Sep 17 00:00:00 2001 From: Markus Date: Sat, 18 Jul 2026 06:50:18 +0200 Subject: [PATCH 20/29] fix(workflows): align layer source validation with _safe_workflow_id_dir MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Plan §4.1 requires that Workflow-ID-Validierung, Symlink-/Containment- Prüfungen and Fehlerübersetzung must not diverge between workflow management and the overlay resolver. My previous fix added ID pattern + reserved-name validation to both collect() methods but was missing the containment step and the BaseWorkflowSource directory/file checks that _safe_workflow_id_dir performs. Changes: - Add _ensure_contained_dir(path, root) to layer_sources.py — pure domain mirror of overlays/_commands.py::_ensure_contained_dir that raises OverlayLoadError instead of typer.Exit - ProjectOverlaySource.collect(): replace two inline symlink/dir checks with _ensure_contained_dir(workflow_overlay_dir, self.overlays_dir), adding the missing resolve().relative_to() containment step - BaseWorkflowSource.collect(): add _ensure_contained_dir on the workflow directory, and add workflow.yml symlink check before is_file() The same logic now lives in three places (workflow CLI, overlay CLI, layer sources). The DRY extraction to workflows/_validation.py is deferred to PR 3 per plan §4.1. Tests: add containment and symlink tests for both sources. Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../workflows/overlays/layer_sources.py | 39 ++++++--- tests/workflows/test_overlay_layer_sources.py | 86 +++++++++++++++++++ 2 files changed, 114 insertions(+), 11 deletions(-) diff --git a/src/specify_cli/workflows/overlays/layer_sources.py b/src/specify_cli/workflows/overlays/layer_sources.py index 5ced4713ab..ecf38814ab 100644 --- a/src/specify_cli/workflows/overlays/layer_sources.py +++ b/src/specify_cli/workflows/overlays/layer_sources.py @@ -47,6 +47,28 @@ def _validate_workflow_id(workflow_id: str, context_path: Path) -> None: ) +def _ensure_contained_dir(path: Path, root: Path) -> None: + """Raise OverlayLoadError if *path* is a symlink, a non-directory, or escapes *root*. + + Mirrors the logic of ``_ensure_contained_dir`` in ``overlays/_commands.py`` + but raises ``OverlayLoadError`` instead of ``typer.Exit`` so layer sources + can enforce the same invariants without a CLI dependency. + + The caller is responsible for ensuring *root* itself is already validated + (e.g. via ``_resolve_project_overlay_root``). + """ + if path.is_symlink(): + raise OverlayLoadError(path, ["Symlinked overlay directories are not allowed"]) + if path.exists() and not path.is_dir(): + raise OverlayLoadError(path, ["Overlay directory path is not a directory"]) + try: + path.resolve().relative_to(root.resolve()) + except ValueError: + raise OverlayLoadError( + path, ["Path traversal detected: directory escapes allowed root"] + ) from None + + def _resolve_project_overlay_root(project_root: Path) -> Path: """Return the unresolved overlay root after rejecting symlinked ancestors. @@ -96,16 +118,7 @@ def collect(self, workflow_id: str) -> list[Layer]: self.overlays_dir = _resolve_project_overlay_root(self.project_root) _validate_workflow_id(workflow_id, self.overlays_dir) workflow_overlay_dir = self.overlays_dir / workflow_id - if workflow_overlay_dir.is_symlink(): - raise OverlayLoadError( - workflow_overlay_dir, - ["Symlinked overlay directories are not allowed"], - ) - if workflow_overlay_dir.exists() and not workflow_overlay_dir.is_dir(): - raise OverlayLoadError( - workflow_overlay_dir, - ["Overlay directory path is not a directory"], - ) + _ensure_contained_dir(workflow_overlay_dir, self.overlays_dir) if not workflow_overlay_dir.is_dir(): return [] layers: list[Layer] = [] @@ -157,7 +170,11 @@ def __init__(self, project_root: Path) -> None: def collect(self, workflow_id: str) -> list[Layer]: """Return the base workflow as a single layer if it exists.""" _validate_workflow_id(workflow_id, self.workflows_dir) - path = self.workflows_dir / workflow_id / "workflow.yml" + workflow_dir = self.workflows_dir / workflow_id + _ensure_contained_dir(workflow_dir, self.workflows_dir) + path = workflow_dir / "workflow.yml" + if path.is_symlink(): + raise OverlayLoadError(path, ["Symlinked workflow files are not allowed"]) if not path.is_file(): return [] # The base layer is represented by an Overlay with empty edits. diff --git a/tests/workflows/test_overlay_layer_sources.py b/tests/workflows/test_overlay_layer_sources.py index 9db33807ba..c8a3ad9617 100644 --- a/tests/workflows/test_overlay_layer_sources.py +++ b/tests/workflows/test_overlay_layer_sources.py @@ -96,6 +96,53 @@ def test_does_not_access_filesystem_for_unsafe_id( source.collect(workflow_id) +class TestProjectOverlaySourceContainment: + """ProjectOverlaySource.collect() must enforce containment of the workflow overlay dir.""" + + def test_rejects_symlinked_workflow_overlay_dir(self, project_dir: Path, tmp_path: Path) -> None: + """A symlinked per-workflow overlay directory must be rejected.""" + real_dir = tmp_path / "real-overlay" + real_dir.mkdir() + overlay_root = project_dir / ".specify" / "workflows" / "overlays" + overlay_root.mkdir(parents=True, exist_ok=True) + link = overlay_root / "wf" + link.symlink_to(real_dir) + + source = ProjectOverlaySource(project_dir) + with pytest.raises(OverlayLoadError, match="Symlinked overlay directories are not allowed"): + source.collect("wf") + + def test_rejects_workflow_overlay_dir_escaping_root( + self, project_dir: Path, tmp_path: Path + ) -> None: + """A workflow overlay dir that resolves outside the overlay root must be rejected. + + This requires the ID itself to pass validation but the resolved path to escape — + which is possible if the overlay root itself is a junction/mount that resolves + outside the project root; or in edge cases on case-insensitive file systems. + We simulate it by patching Path.resolve to return an outside path. + """ + overlay_root = project_dir / ".specify" / "workflows" / "overlays" + overlay_root.mkdir(parents=True, exist_ok=True) + workflow_overlay_dir = overlay_root / "wf" + workflow_overlay_dir.mkdir() + + outside = tmp_path / "outside" / "wf" + outside.mkdir(parents=True) + + original_resolve = Path.resolve + + def fake_resolve(self: Path, **kwargs: object) -> Path: + if self == workflow_overlay_dir: + return outside + return original_resolve(self, **kwargs) + + source = ProjectOverlaySource(project_dir) + with patch.object(Path, "resolve", fake_resolve): + with pytest.raises(OverlayLoadError, match="Path traversal detected"): + source.collect("wf") + + class TestBaseWorkflowSourceIdValidation: """BaseWorkflowSource.collect() must reject unsafe IDs before path construction.""" @@ -105,3 +152,42 @@ def test_rejects_unsafe_id(self, project_dir: Path, workflow_id: str) -> None: with pytest.raises(OverlayLoadError, match="Invalid workflow ID"): source.collect(workflow_id) + +class TestBaseWorkflowSourceContainment: + """BaseWorkflowSource.collect() must enforce the same checks as _safe_workflow_id_dir.""" + + def test_rejects_symlinked_workflow_dir(self, project_dir: Path, tmp_path: Path) -> None: + """A symlinked workflow directory must be rejected.""" + real_dir = tmp_path / "real-wf" + real_dir.mkdir() + (real_dir / "workflow.yml").write_text("schema_version: '1.0'\n", encoding="utf-8") + + workflows_dir = project_dir / ".specify" / "workflows" + workflows_dir.mkdir(parents=True, exist_ok=True) + link = workflows_dir / "wf" + link.symlink_to(real_dir) + + source = BaseWorkflowSource(project_dir) + with pytest.raises(OverlayLoadError, match="Symlinked overlay directories are not allowed"): + source.collect("wf") + + def test_rejects_symlinked_workflow_yml(self, project_dir: Path, tmp_path: Path) -> None: + """A symlinked workflow.yml must be rejected even if the directory is real.""" + real_yml = tmp_path / "workflow.yml" + real_yml.write_text("schema_version: '1.0'\n", encoding="utf-8") + + workflows_dir = project_dir / ".specify" / "workflows" + wf_dir = workflows_dir / "wf" + wf_dir.mkdir(parents=True, exist_ok=True) + link = wf_dir / "workflow.yml" + link.symlink_to(real_yml) + + source = BaseWorkflowSource(project_dir) + with pytest.raises(OverlayLoadError, match="Symlinked workflow files are not allowed"): + source.collect("wf") + + def test_missing_workflow_returns_empty(self, project_dir: Path) -> None: + """A workflow directory that does not exist returns an empty layer list.""" + source = BaseWorkflowSource(project_dir) + assert source.collect("no-such-wf") == [] + From 9ebfc3a93112b91ae0b0c3aa9d6ebd30f557d089 Mon Sep 17 00:00:00 2001 From: Markus Wondrak Date: Sat, 18 Jul 2026 06:51:33 +0200 Subject: [PATCH 21/29] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/specify_cli/workflows/overlays/merge.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/specify_cli/workflows/overlays/merge.py b/src/specify_cli/workflows/overlays/merge.py index a1a6fdd607..d818169e82 100644 --- a/src/specify_cli/workflows/overlays/merge.py +++ b/src/specify_cli/workflows/overlays/merge.py @@ -188,7 +188,7 @@ def _remove_sources_recursively( Traverses the same nesting keys as ``_record_sources_recursively``. """ step_id = step.get("id") - if isinstance(step_id, str): + if isinstance(step_id, str) and sources.get(step_id) == "base": sources.pop(step_id, None) for key in _NESTED_LIST_KEYS: nested = step.get(key) From e70bbb769be0eebafecc263a20156dbc31050066 Mon Sep 17 00:00:00 2001 From: Markus Date: Sat, 18 Jul 2026 07:27:35 +0200 Subject: [PATCH 22/29] fix(overlays): resolve identity from manifest field, not filename Align overlay identity resolution with the project-wide convention: presets use preset.id, extensions use extension.id, workflows use workflow.id, and workflow steps use step.type_key. Overlays must derive identity from the manifest id field, not the filename. Rewrite _find_overlay_file() to scan all YAML files in the overlay directory and match on the manifest id field, fixing the bug where enable/disable/remove/set-priority failed when filename != manifest id. Closes: PR #3557 discussion r3605010197 Assisted-by: opencode-go/qwen3.7-max (autonomous) --- .../workflows/overlays/_commands.py | 33 ++- tests/workflows/test_overlay_commands.py | 209 ++++++++++++++++++ 2 files changed, 231 insertions(+), 11 deletions(-) diff --git a/src/specify_cli/workflows/overlays/_commands.py b/src/specify_cli/workflows/overlays/_commands.py index 4febb73d66..32b0597eb8 100644 --- a/src/specify_cli/workflows/overlays/_commands.py +++ b/src/specify_cli/workflows/overlays/_commands.py @@ -86,20 +86,31 @@ def _ensure_contained_dir(path: Path, root: Path) -> Path: def _find_overlay_file(project_root: Path, workflow_id: str, overlay_id: str) -> Path | None: - """Locate a project-local overlay file by workflow id and overlay id.""" + """Locate a project-local overlay file by its manifest ID, not filename. + + Scans all YAML files in the overlay directory and matches on the ``id`` + field inside each manifest. This aligns with ``ProjectOverlaySource.collect()`` + which also derives identity from the manifest, not the filename. + """ _validate_workflow_id_or_exit(workflow_id) _validate_overlay_id_or_exit(overlay_id, "overlay ID") overlay_dir = _project_overlay_dir(project_root, workflow_id) - for suffix in (".yml", ".yaml"): - candidate = overlay_dir / f"{overlay_id}{suffix}" - candidate = _ensure_contained_path(candidate, _overlay_root(project_root)) - if candidate.is_file(): - if candidate.is_symlink(): - err_console.print( - f"[red]Error:[/red] Refusing to read symlinked overlay file {candidate}." - ) - raise typer.Exit(1) - return candidate + if not overlay_dir.is_dir(): + return None + try: + entries = sorted(overlay_dir.iterdir()) + except OSError: + return None + for path in entries: + if not path.is_file() or path.suffix not in (".yml", ".yaml"): + continue + if path.is_symlink(): + continue + data, _ = _read_overlay(path) + if data is None: + continue + if data.get("id") == overlay_id: + return path return None diff --git a/tests/workflows/test_overlay_commands.py b/tests/workflows/test_overlay_commands.py index c5a11aa1f3..6b9cc90d4a 100644 --- a/tests/workflows/test_overlay_commands.py +++ b/tests/workflows/test_overlay_commands.py @@ -507,3 +507,212 @@ def test_workflow_add_does_not_copy_overlays(self, project_dir, monkeypatch, tmp project_dir / ".specify" / "workflows" / "wf" / "overlays" / "ov1.yml" ) assert not installed_overlay.exists() + + +class TestOverlayFilenameVsManifestId: + """Overlay identity must come from the manifest ``id`` field, not the filename. + + This matches the project-wide convention: presets use ``preset.id``, + extensions use ``extension.id``, workflows use ``workflow.id``, and + workflow steps use ``step.type_key``. Overlays must follow the same pattern. + """ + + def _write_mismatched_overlay( + self, project_root: Path, workflow_id: str, filename: str, manifest_id: str, data: dict + ) -> Path: + """Write an overlay file where filename != manifest id.""" + ov_dir = project_root / ".specify" / "workflows" / "overlays" / workflow_id + ov_dir.mkdir(parents=True, exist_ok=True) + ov_path = ov_dir / filename + ov_path.write_text(yaml.safe_dump(data), encoding="utf-8") + return ov_path + + def test_find_overlay_by_manifest_id_not_filename(self, project_dir, monkeypatch): + """_find_overlay_file must locate overlays by manifest id, not filename.""" + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + _write_workflow( + project_dir, + "wf", + { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "echo"}], + }, + ) + # File is named "custom.yml" but manifest declares id: "lint" + self._write_mismatched_overlay( + project_dir, + "wf", + "custom.yml", + "lint", + { + "id": "lint", + "extends": "wf", + "priority": 10, + "edits": [{"remove": "a"}], + }, + ) + + from specify_cli.workflows.overlays._commands import _find_overlay_file + + # Must find by manifest id "lint", not by filename "custom" + found = _find_overlay_file(project_dir, "wf", "lint") + assert found is not None + assert found.name == "custom.yml" + + # Must NOT find by filename stem "custom" + not_found = _find_overlay_file(project_dir, "wf", "custom") + assert not_found is None + + def test_enable_disable_with_mismatched_filename(self, project_dir, monkeypatch): + """enable/disable must work when filename != manifest id.""" + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + _write_workflow( + project_dir, + "wf", + { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "echo"}], + }, + ) + self._write_mismatched_overlay( + project_dir, + "wf", + "custom.yml", + "lint", + { + "id": "lint", + "extends": "wf", + "priority": 10, + "edits": [{"remove": "a"}], + }, + ) + + result = runner.invoke(app, ["workflow", "overlay", "disable", "wf", "lint"]) + assert result.exit_code == 0, result.output + data = yaml.safe_load( + (project_dir / ".specify" / "workflows" / "overlays" / "wf" / "custom.yml").read_text( + encoding="utf-8" + ) + ) + assert data["enabled"] is False + + result = runner.invoke(app, ["workflow", "overlay", "enable", "wf", "lint"]) + assert result.exit_code == 0, result.output + data = yaml.safe_load( + (project_dir / ".specify" / "workflows" / "overlays" / "wf" / "custom.yml").read_text( + encoding="utf-8" + ) + ) + assert data["enabled"] is True + + def test_set_priority_with_mismatched_filename(self, project_dir, monkeypatch): + """set-priority must work when filename != manifest id.""" + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + _write_workflow( + project_dir, + "wf", + { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "echo"}], + }, + ) + self._write_mismatched_overlay( + project_dir, + "wf", + "custom.yml", + "lint", + { + "id": "lint", + "extends": "wf", + "priority": 10, + "edits": [{"remove": "a"}], + }, + ) + + result = runner.invoke(app, ["workflow", "overlay", "set-priority", "wf", "lint", "25"]) + assert result.exit_code == 0, result.output + data = yaml.safe_load( + (project_dir / ".specify" / "workflows" / "overlays" / "wf" / "custom.yml").read_text( + encoding="utf-8" + ) + ) + assert data["priority"] == 25 + + def test_remove_with_mismatched_filename(self, project_dir, monkeypatch): + """remove must work when filename != manifest id.""" + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + _write_workflow( + project_dir, + "wf", + { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "echo"}], + }, + ) + self._write_mismatched_overlay( + project_dir, + "wf", + "custom.yml", + "lint", + { + "id": "lint", + "extends": "wf", + "priority": 10, + "edits": [{"remove": "a"}], + }, + ) + + result = runner.invoke(app, ["workflow", "overlay", "remove", "wf", "lint"]) + assert result.exit_code == 0, result.output + assert not ( + project_dir / ".specify" / "workflows" / "overlays" / "wf" / "custom.yml" + ).exists() + + def test_duplicate_manifest_id_returns_first_sorted(self, project_dir, monkeypatch): + """Two files with the same manifest id: first sorted match wins.""" + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + _write_workflow( + project_dir, + "wf", + { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "echo"}], + }, + ) + # Two files, both declare id: "lint" + self._write_mismatched_overlay( + project_dir, + "wf", + "aaa.yml", + "lint", + { + "id": "lint", + "extends": "wf", + "priority": 10, + "edits": [{"remove": "a"}], + }, + ) + self._write_mismatched_overlay( + project_dir, + "wf", + "zzz.yml", + "lint", + { + "id": "lint", + "extends": "wf", + "priority": 20, + "edits": [{"remove": "a"}], + }, + ) + + from specify_cli.workflows.overlays._commands import _find_overlay_file + + found = _find_overlay_file(project_dir, "wf", "lint") + assert found is not None + # sorted() returns aaa.yml before zzz.yml + assert found.name == "aaa.yml" From 79c5b9383d63a694d2528d30e3c4701522b561f2 Mon Sep 17 00:00:00 2001 From: Markus Wondrak Date: Sat, 18 Jul 2026 07:29:40 +0200 Subject: [PATCH 23/29] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/specify_cli/workflows/overlays/layer_sources.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/workflows/overlays/layer_sources.py b/src/specify_cli/workflows/overlays/layer_sources.py index 0b631a6db9..c64d259a1f 100644 --- a/src/specify_cli/workflows/overlays/layer_sources.py +++ b/src/specify_cli/workflows/overlays/layer_sources.py @@ -108,7 +108,13 @@ def collect(self, workflow_id: str) -> list[Layer]: if not overlay.enabled: continue if overlay.extends != workflow_id: - continue + raise OverlayLoadError( + path, + [ + f"Overlay extends {overlay.extends!r}, but is stored under " + f"workflow {workflow_id!r}." + ], + ) layers.append( Layer( content=overlay, From 901a7c857fd5c8f605398ca03e1e143d23879f86 Mon Sep 17 00:00:00 2001 From: Markus Date: Sat, 18 Jul 2026 08:40:23 +0200 Subject: [PATCH 24/29] fix(workflows): make validation behavior consistent across YAML loading paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR #3557 review finding r3607632921: - Wrap yaml.YAMLError → ValueError in from_yaml() and from_string() so malformed YAML matches the documented exception contract - Add except ValueError to workflow_info to handle composition errors cleanly instead of crashing with a raw traceback - Remove validate_workflow() from compose() so the resolver path is parse-only like all other YAML loading mechanisms; callers validate explicitly via engine.validate() - Update test to reflect new behavior: resolve() returns composed definition, caller validates separately Assisted-by: opencode-go/qwen3.7-max (autonomous) --- src/specify_cli/workflows/_commands.py | 3 +++ src/specify_cli/workflows/engine.py | 12 ++++++++++-- src/specify_cli/workflows/overlays/composer.py | 11 ++--------- tests/workflows/test_resolver_integration.py | 9 +++++---- 4 files changed, 20 insertions(+), 15 deletions(-) diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index 2ccd8061e5..01c457afdb 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -2397,6 +2397,9 @@ def workflow_info( # Local workflow definition not found on disk; fall back to # catalog/registry lookup below. pass + except ValueError as exc: + console.print(f"[red]Error:[/red] Invalid workflow: {_escape_markup(str(exc))}") + raise typer.Exit(1) if definition: console.print(f"\n[bold cyan]{definition.name}[/bold cyan] ({definition.id})") diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index c3987a1b04..6232fc951a 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -79,7 +79,11 @@ def __init__(self, data: dict[str, Any], source_path: Path | None = None) -> Non def from_yaml(cls, path: Path) -> WorkflowDefinition: """Load a workflow definition from a YAML file.""" with open(path, encoding="utf-8") as f: - data = yaml.safe_load(f) + try: + data = yaml.safe_load(f) + except yaml.YAMLError as exc: + msg = f"Invalid YAML in {path}: {exc}" + raise ValueError(msg) from exc if not isinstance(data, dict): msg = f"Workflow YAML must be a mapping, got {type(data).__name__}." raise ValueError(msg) @@ -88,7 +92,11 @@ def from_yaml(cls, path: Path) -> WorkflowDefinition: @classmethod def from_string(cls, content: str) -> WorkflowDefinition: """Load a workflow definition from a YAML string.""" - data = yaml.safe_load(content) + try: + data = yaml.safe_load(content) + except yaml.YAMLError as exc: + msg = f"Invalid YAML: {exc}" + raise ValueError(msg) from exc if not isinstance(data, dict): msg = f"Workflow YAML must be a mapping, got {type(data).__name__}." raise ValueError(msg) diff --git a/src/specify_cli/workflows/overlays/composer.py b/src/specify_cli/workflows/overlays/composer.py index 8ce3d16440..a09277fd20 100644 --- a/src/specify_cli/workflows/overlays/composer.py +++ b/src/specify_cli/workflows/overlays/composer.py @@ -4,7 +4,7 @@ from typing import Any -from ..engine import WorkflowDefinition, validate_workflow +from ..engine import WorkflowDefinition from .layer_sources import Layer from .merge import OverlayLayer, merge_steps, validate_edits @@ -17,7 +17,7 @@ class StepListComposer: - Overlays are applied in merge order: lowest priority first, highest last; ties are resolved so installed overlays are applied before project overlays, letting project overlays win. - - ``validate_workflow`` runs on the composed result. + - Returns a parsed WorkflowDefinition; callers must validate separately. """ def compose( @@ -74,13 +74,6 @@ def compose( composed_data["steps"] = composed_steps composed_definition = WorkflowDefinition(composed_data, source_path=base_layer.path) - validation_errors = validate_workflow(composed_definition) - if validation_errors: - # Surface validation errors as a single exception with details. - raise ValueError( - "Composed workflow is invalid:\n - " - + "\n - ".join(validation_errors) - ) return composed_definition, attribution diff --git a/tests/workflows/test_resolver_integration.py b/tests/workflows/test_resolver_integration.py index a007fdb55a..1dc8ee5a92 100644 --- a/tests/workflows/test_resolver_integration.py +++ b/tests/workflows/test_resolver_integration.py @@ -7,7 +7,7 @@ import pytest import yaml -from specify_cli.workflows.engine import WorkflowDefinition +from specify_cli.workflows.engine import WorkflowDefinition, validate_workflow from specify_cli.workflows.overlays import WorkflowResolver from specify_cli.workflows.overlays.merge import ComposedStep @@ -307,7 +307,7 @@ def test_resolve_missing_workflow(self, project_dir): with pytest.raises(FileNotFoundError, match="Workflow not found"): resolver.resolve("missing") - def test_resolve_validates_composed_result(self, project_dir): + def test_resolve_returns_composed_result_for_caller_validation(self, project_dir): data = { "schema_version": "1.0", "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, @@ -333,8 +333,9 @@ def test_resolve_validates_composed_result(self, project_dir): ) resolver = WorkflowResolver(project_dir) - with pytest.raises(ValueError, match="Composed workflow is invalid"): - resolver.resolve("wf") + definition = resolver.resolve("wf") + errors = validate_workflow(definition) + assert any("invalid-type" in err for err in errors) def test_resolve_rejects_symlinked_project_overlay_dir(self, project_dir, tmp_path): """ProjectOverlaySource must reject a symlinked per-workflow overlay directory.""" From 1e44f13308285fa78875b07057e2632d6ed45171 Mon Sep 17 00:00:00 2001 From: Markus Date: Sat, 18 Jul 2026 08:54:48 +0200 Subject: [PATCH 25/29] fix(overlays): list disabled overlays in management view Keep disabled overlays visible in workflow overlay list while leaving resolution behavior unchanged. - add an include_disabled opt-in to overlay source/resolver collection - use include_disabled=True for workflow overlay list - add regression tests for list visibility and default filtering Assisted-by: GitHub Copilot (model: GPT-5.4, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../workflows/overlays/__init__.py | 8 +++- .../workflows/overlays/_commands.py | 2 +- .../workflows/overlays/layer_sources.py | 14 +++++-- tests/workflows/test_overlay_commands.py | 35 ++++++++++++++++ tests/workflows/test_overlay_layer_sources.py | 41 ++++++++++++++++++- tests/workflows/test_resolver_integration.py | 33 +++++++++++++++ 6 files changed, 125 insertions(+), 8 deletions(-) diff --git a/src/specify_cli/workflows/overlays/__init__.py b/src/specify_cli/workflows/overlays/__init__.py index 017645560f..99767116ca 100644 --- a/src/specify_cli/workflows/overlays/__init__.py +++ b/src/specify_cli/workflows/overlays/__init__.py @@ -44,7 +44,9 @@ def __init__(self, project_root: Path) -> None: ] self._composer = StepListComposer() - def collect_all_layers(self, workflow_id: str) -> list[Layer]: + def collect_all_layers( + self, workflow_id: str, *, include_disabled: bool = False + ) -> list[Layer]: """Collect all layers sorted by resolver precedence. Higher priority wins. Ties are broken so the actual winner appears first: @@ -56,7 +58,9 @@ def collect_all_layers(self, workflow_id: str) -> list[Layer]: all_layers: list[Layer] = [] for source in self._sources: - all_layers.extend(source.collect(workflow_id)) + all_layers.extend( + source.collect(workflow_id, include_disabled=include_disabled) + ) # Pass 1: source descending — within each priority group the winning # source (alphabetically last = applied last by the composer) rises first. diff --git a/src/specify_cli/workflows/overlays/_commands.py b/src/specify_cli/workflows/overlays/_commands.py index 32b0597eb8..44ae3fde28 100644 --- a/src/specify_cli/workflows/overlays/_commands.py +++ b/src/specify_cli/workflows/overlays/_commands.py @@ -323,7 +323,7 @@ def workflow_overlay_list(project_root: Path, workflow_id: str) -> list[dict[str _validate_workflow_id_or_exit(workflow_id) resolver = WorkflowResolver(project_root) try: - layers = resolver.collect_all_layers(workflow_id) + layers = resolver.collect_all_layers(workflow_id, include_disabled=True) except ValueError as exc: err_console.print(f"[red]Error:[/red] {exc}") return None diff --git a/src/specify_cli/workflows/overlays/layer_sources.py b/src/specify_cli/workflows/overlays/layer_sources.py index 80cf1e913d..fea3a10993 100644 --- a/src/specify_cli/workflows/overlays/layer_sources.py +++ b/src/specify_cli/workflows/overlays/layer_sources.py @@ -113,8 +113,14 @@ def __init__(self, project_root: Path) -> None: self.project_root = project_root self.overlays_dir = project_root / ".specify" / "workflows" / "overlays" - def collect(self, workflow_id: str) -> list[Layer]: - """Collect all project-local overlays for the given workflow id.""" + def collect(self, workflow_id: str, *, include_disabled: bool = False) -> list[Layer]: + """Collect project-local overlays for the given workflow id. + + Args: + workflow_id: Workflow identifier whose overlay directory to scan. + include_disabled: When True, return disabled overlays for + management/list views. Resolution paths keep the default False. + """ self.overlays_dir = _resolve_project_overlay_root(self.project_root) _validate_workflow_id(workflow_id, self.overlays_dir) workflow_overlay_dir = self.overlays_dir / workflow_id @@ -142,7 +148,7 @@ def collect(self, workflow_id: str) -> list[Layer]: overlay, errors = validate_overlay_yaml(data) if overlay is None or errors: raise OverlayLoadError(path, errors) - if not overlay.enabled: + if not include_disabled and not overlay.enabled: continue if overlay.extends != workflow_id: raise OverlayLoadError( @@ -173,7 +179,7 @@ def __init__(self, project_root: Path) -> None: self.project_root = project_root self.workflows_dir = project_root / ".specify" / "workflows" - def collect(self, workflow_id: str) -> list[Layer]: + def collect(self, workflow_id: str, *, include_disabled: bool = False) -> list[Layer]: """Return the base workflow as a single layer if it exists.""" _validate_workflow_id(workflow_id, self.workflows_dir) workflow_dir = self.workflows_dir / workflow_id diff --git a/tests/workflows/test_overlay_commands.py b/tests/workflows/test_overlay_commands.py index 6b9cc90d4a..db98a3bcba 100644 --- a/tests/workflows/test_overlay_commands.py +++ b/tests/workflows/test_overlay_commands.py @@ -372,6 +372,41 @@ def test_overlay_list(self, project_dir, monkeypatch): assert result.exit_code == 0, result.output assert "ov1" in result.output + def test_overlay_list_shows_disabled_overlay(self, project_dir, monkeypatch): + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + _write_workflow( + project_dir, + "wf", + { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "echo"}], + }, + ) + _write_overlay( + project_dir, + "wf", + "ov1", + { + "id": "ov1", + "extends": "wf", + "priority": 10, + "enabled": False, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "new", "type": "command", "command": "echo"}, + } + ], + }, + ) + + result = runner.invoke(app, ["workflow", "overlay", "list", "wf"]) + assert result.exit_code == 0, result.output + assert "ov1" in result.output + assert "disabled" in result.output + def test_workflow_resolve(self, project_dir, monkeypatch): monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) _write_workflow( diff --git a/tests/workflows/test_overlay_layer_sources.py b/tests/workflows/test_overlay_layer_sources.py index c8a3ad9617..f3cc9b305d 100644 --- a/tests/workflows/test_overlay_layer_sources.py +++ b/tests/workflows/test_overlay_layer_sources.py @@ -143,6 +143,46 @@ def fake_resolve(self: Path, **kwargs: object) -> Path: source.collect("wf") +class TestProjectOverlaySourceDisabledFiltering: + """ProjectOverlaySource.collect() should expose disabled entries only on opt-in.""" + + def test_skips_disabled_by_default(self, project_dir: Path) -> None: + _write_overlay_file( + project_dir, + "wf", + "ov1", + { + "id": "ov1", + "extends": "wf", + "priority": 5, + "enabled": False, + "edits": [{"remove": "a"}], + }, + ) + + source = ProjectOverlaySource(project_dir) + assert source.collect("wf") == [] + + def test_can_include_disabled_for_management_views(self, project_dir: Path) -> None: + _write_overlay_file( + project_dir, + "wf", + "ov1", + { + "id": "ov1", + "extends": "wf", + "priority": 5, + "enabled": False, + "edits": [{"remove": "a"}], + }, + ) + + source = ProjectOverlaySource(project_dir) + layers = source.collect("wf", include_disabled=True) + assert [layer.content.id for layer in layers] == ["ov1"] + assert layers[0].content.enabled is False + + class TestBaseWorkflowSourceIdValidation: """BaseWorkflowSource.collect() must reject unsafe IDs before path construction.""" @@ -190,4 +230,3 @@ def test_missing_workflow_returns_empty(self, project_dir: Path) -> None: """A workflow directory that does not exist returns an empty layer list.""" source = BaseWorkflowSource(project_dir) assert source.collect("no-such-wf") == [] - diff --git a/tests/workflows/test_resolver_integration.py b/tests/workflows/test_resolver_integration.py index 1dc8ee5a92..aca426a73f 100644 --- a/tests/workflows/test_resolver_integration.py +++ b/tests/workflows/test_resolver_integration.py @@ -273,6 +273,39 @@ def test_resolve_disabled_overlay_is_skipped(self, project_dir): definition = resolver.resolve("wf") assert [s["id"] for s in definition.steps] == ["a"] + def test_collect_all_layers_can_include_disabled_overlay_for_listing(self, project_dir): + data = { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "speckit.specify"}], + } + _write_workflow(project_dir, "wf", data) + _write_overlay( + project_dir, + "wf", + "disabled", + { + "id": "disabled", + "extends": "wf", + "priority": 10, + "enabled": False, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "new", "type": "command", "command": "echo"}, + } + ], + }, + ) + + resolver = WorkflowResolver(project_dir) + default_layers = resolver.collect_all_layers("wf") + listed_layers = resolver.collect_all_layers("wf", include_disabled=True) + + assert [layer.source for layer in default_layers] == ["base"] + assert [layer.source for layer in listed_layers] == ["project:disabled", "base"] + def test_resolve_invalid_anchor_raises(self, project_dir): data = { "schema_version": "1.0", From fc9d7103024dc008dc8bd650b5162b5d6410c813 Mon Sep 17 00:00:00 2001 From: Markus Wondrak Date: Sat, 18 Jul 2026 17:35:47 +0200 Subject: [PATCH 26/29] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- docs/reference/workflows.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index 0097f8f62e..0af69b5ec3 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -185,7 +185,7 @@ Validates the overlay file and copies it to `.specify/workflows/overlays/ ``` -Shows enabled overlays for the workflow, ordered by resolver precedence. Disabled overlays are ignored by the resolver and are not listed. +Shows all overlays for the workflow, ordered by resolver precedence. Disabled overlays are marked as disabled in the listing and are ignored during workflow resolution. #### Change Priority From 08fa12ec37c53111314d981503de44ec98ccd7f5 Mon Sep 17 00:00:00 2001 From: Markus Date: Sat, 18 Jul 2026 17:37:19 +0200 Subject: [PATCH 27/29] docs: align overlay extends and resolver contract Assisted-by: GitHub Copilot (model: GPT-5.3-Codex, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/reference/workflows.md | 2 +- src/specify_cli/workflows/overlays/__init__.py | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index 0af69b5ec3..30b60b3bcf 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -151,7 +151,7 @@ edits: | Field | Required | Description | | --- | --- | --- | | `id` | yes | Identifier for this overlay. Used in `specify workflow overlay *` commands. Must be lowercase letters, digits, and hyphens only; no dots, underscores, path separators, or `overlays`. | -| `extends` | yes | The workflow id this overlay applies to. Uses the same safe-id format as `id`; `overlays` is reserved. | +| `extends` | yes | The workflow id this overlay applies to. Uses the same safe-id format as `id`; `overlays`, `runs`, and `steps` are reserved. | | `priority` | yes | Integer `>= 1`. Higher priority overlays are applied later and win conflicts. | | `enabled` | no | Boolean. Defaults to `true`. Disabled overlays are ignored. | | `edits` | yes | Non-empty list of edit operations. | diff --git a/src/specify_cli/workflows/overlays/__init__.py b/src/specify_cli/workflows/overlays/__init__.py index 99767116ca..195588185d 100644 --- a/src/specify_cli/workflows/overlays/__init__.py +++ b/src/specify_cli/workflows/overlays/__init__.py @@ -71,9 +71,12 @@ def collect_all_layers( def resolve(self, workflow_id: str) -> WorkflowDefinition: """Resolve a workflow ID to its composed definition. + This method composes layers but does not validate workflow semantics; + callers should validate the returned definition when needed. + Raises: FileNotFoundError: if the workflow cannot be found. - ValueError: if the composed workflow is invalid. + ValueError: if layer collection/composition fails. """ layers = self.collect_all_layers(workflow_id) definition, _ = self._composer.compose(layers) From 54c5b9f993be72049873f641a4db3c4a5fb7056d Mon Sep 17 00:00:00 2001 From: Markus Date: Sat, 18 Jul 2026 19:34:20 +0200 Subject: [PATCH 28/29] fix: use atomic write for overlay file updates to prevent hard-link attack MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace in-place write_text() calls in workflow_overlay_add() and _update_overlay_field() with the same mkstemp → write → os.replace() pattern used by the workflow installer (_stage_workflow_file / _commit_workflow_file / _discard_staged_workflow_file). The prior code rejected symlinks and validated path containment, but a hard-linked destination file passes both checks while sharing an inode with an external file. write_text() would then truncate and overwrite that external inode. The atomic staging approach never opens the existing destination for writing, eliminating the hard-link vector. Fixes findings r3608669512 and r3608669517 on PR #3557. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, supervised) --- .../workflows/overlays/_commands.py | 26 ++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/src/specify_cli/workflows/overlays/_commands.py b/src/specify_cli/workflows/overlays/_commands.py index 44ae3fde28..168dfdd494 100644 --- a/src/specify_cli/workflows/overlays/_commands.py +++ b/src/specify_cli/workflows/overlays/_commands.py @@ -9,7 +9,13 @@ import yaml from ..._console import console, err_console -from .._commands import _reject_unsafe_dir, _reject_unsafe_workflow_storage +from .._commands import ( + _commit_workflow_file, + _discard_staged_workflow_file, + _reject_unsafe_dir, + _reject_unsafe_workflow_storage, + _stage_workflow_file, +) from . import WorkflowResolver from .schema import _RESERVED_WORKFLOW_IDS, _SAFE_ID_PATTERN, validate_overlay_yaml @@ -200,7 +206,14 @@ def workflow_overlay_add( ) try: - target_path.write_text(yaml.safe_dump(data, sort_keys=False), encoding="utf-8") + existed_before = target_path.exists() + staged = _stage_workflow_file(target_path.parent) + try: + staged.write_bytes(yaml.safe_dump(data, sort_keys=False).encode("utf-8")) + _commit_workflow_file(staged, target_path, existed_before) + except BaseException: + _discard_staged_workflow_file(staged, target_path.parent) + raise except OSError as exc: err_console.print(f"[red]Error:[/red] Failed to write overlay: {exc}") return None @@ -242,7 +255,14 @@ def _update_overlay_field( return False try: - path.write_text(yaml.safe_dump(data, sort_keys=False), encoding="utf-8") + existed_before = path.exists() + staged = _stage_workflow_file(path.parent) + try: + staged.write_bytes(yaml.safe_dump(data, sort_keys=False).encode("utf-8")) + _commit_workflow_file(staged, path, existed_before) + except BaseException: + _discard_staged_workflow_file(staged, path.parent) + raise except OSError as exc: err_console.print(f"[red]Error:[/red] Failed to write overlay: {exc}") return False From 3f047c9ab3449de193ff6c9dba9d6c7c2e21f2dc Mon Sep 17 00:00:00 2001 From: Markus Date: Sat, 18 Jul 2026 19:40:41 +0200 Subject: [PATCH 29/29] fix(composer): preserve invalid base definition instead of coercing steps to [] When 'steps' is not a list, returning early with the unmodified WorkflowDefinition lets validate_workflow surface the proper error ("'steps' must be a list.") to the caller. The previous silent coercion to [] masked the validation error entirely. Fixes: https://github.com/github/spec-kit/pull/3557#discussion_r3608669506 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, supervised) --- src/specify_cli/workflows/overlays/composer.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/workflows/overlays/composer.py b/src/specify_cli/workflows/overlays/composer.py index a09277fd20..8be645d72e 100644 --- a/src/specify_cli/workflows/overlays/composer.py +++ b/src/specify_cli/workflows/overlays/composer.py @@ -42,7 +42,10 @@ def compose( base_definition = WorkflowDefinition.from_yaml(base_layer.path) base_steps = base_definition.data.get("steps", []) if not isinstance(base_steps, list): - base_steps = [] + # Preserve the invalid definition intact so validate_workflow can + # report "'steps' must be a list." to the caller; coercing to [] + # here would mask that error. + return base_definition, [] # Sort overlays into merge order. merge_order = sorted(