From 17496f28ab2084de536f180c0955346bbb294770 Mon Sep 17 00:00:00 2001 From: Markus Date: Sat, 7 Nov 2026 08:39:24 +0100 Subject: [PATCH 01/11] refactor(workflows): bound filter arguments and add a custom-filter registry Replace FilterSpec.argument/bare with min_args/max_args and derive call-form validation from one call_form_ok() shared by evaluation and static validation. Built-in diagnostics keep their hand-written usage text byte for byte. Add an immutable BUILTIN_FILTERS snapshot and register_custom_filter / unregister_custom_filter(s) over the process-global FILTER_TABLE. Custom handlers are called as handler(value, *args) and ordinary exceptions are normalized to ValueError prefixed with the filter name. The expected-filters sentence is built from the live table only when an error is raised. Refs #5285 Assisted-by: pi coding agent (GitHub Copilot, model: claude-sonnet-5.5, autonomous) --- .../workflows/expressions/__init__.py | 16 +- .../workflows/expressions/engine.py | 45 +- .../workflows/expressions/filters.py | 142 +++++-- .../workflows/function/__init__.py | 0 .../workflows/function/conftest.py | 18 + .../workflows/function/test_custom_filters.py | 385 ++++++++++++++++++ 6 files changed, 541 insertions(+), 65 deletions(-) create mode 100644 tests/specify_cli/workflows/function/__init__.py create mode 100644 tests/specify_cli/workflows/function/conftest.py create mode 100644 tests/specify_cli/workflows/function/test_custom_filters.py diff --git a/src/specify_cli/workflows/expressions/__init__.py b/src/specify_cli/workflows/expressions/__init__.py index d69b2878e2..e7af5e0806 100644 --- a/src/specify_cli/workflows/expressions/__init__.py +++ b/src/specify_cli/workflows/expressions/__init__.py @@ -36,7 +36,15 @@ _evaluator_rejects, _has_incomplete_operand, ) -from .filters import _REGISTERED_FILTERS +from .filters import ( + BUILTIN_FILTERS, + FILTER_TABLE, + _REGISTERED_FILTERS, + format_filter_usage, + register_custom_filter, + unregister_custom_filter, + unregister_custom_filters, +) from .template import _strip_stray_delimiters __all__ = [ @@ -49,6 +57,12 @@ "format_condition_remediation", "_COMPARISON_OPERATORS", "_REGISTERED_FILTERS", + "BUILTIN_FILTERS", + "FILTER_TABLE", + "format_filter_usage", + "register_custom_filter", + "unregister_custom_filter", + "unregister_custom_filters", "_WORD_OPERATORS", "_collect_leaves", "_evaluator_rejects", diff --git a/src/specify_cli/workflows/expressions/engine.py b/src/specify_cli/workflows/expressions/engine.py index 0fc93ac7c2..9c9bb48e61 100644 --- a/src/specify_cli/workflows/expressions/engine.py +++ b/src/specify_cli/workflows/expressions/engine.py @@ -48,7 +48,7 @@ from lark.exceptions import UnexpectedCharacters, UnexpectedInput, UnexpectedToken, VisitError from . import namespace -from .filters import FILTER_TABLE, _EXPECTED_FILTERS +from .filters import FILTER_TABLE, _expected_filters, call_form_ok from .namespace import _resolve_dot_path from .operators import _safe_compare, _safe_membership from .template import ( @@ -368,9 +368,9 @@ def _apply_filter( evaluated, so an unknown or misused filter is reported as such even when an argument would itself fail. The diagnostics walk the parse tree for argument leaves, so they do not need the arguments evaluated first. The - error wording comes from ``FILTER_TABLE`` and ``_EXPECTED_FILTERS``: the - same table the message lists, so adding a filter cannot leave the two - disagreeing. + error wording comes from the live ``FILTER_TABLE`` through + ``_expected_filters()``: the same table the message lists, so adding a + filter cannot leave the two disagreeing. """ _, name, arg_nodes, has_parens = filt display = f"{name}(...)" if has_parens else name @@ -378,25 +378,22 @@ def _apply_filter( spec = FILTER_TABLE.get(name) if spec is None: raise ValueError( - f"unknown filter '{name}': {_EXPECTED_FILTERS} (got '| {display}')" + f"unknown filter '{name}': {_expected_filters()} (got '| {display}')" ) - if not spec.argument: + if has_parens and spec.max_args == 0: # A zero-argument filter tolerates no parentheses and no trailing # tokens; `from_json()` and `from_json('x')` are the same mistake. - if has_parens: - raise ValueError( - f"{name}: expected '| {name}' with no arguments or trailing " - f"tokens, got '| {display}'" - ) - return spec.function(value) - if has_parens and len(arg_nodes) == 1: - return spec.function(value, _eval(arg_nodes[0], namespace_)) - if not has_parens and spec.bare: - return spec.function(value) - raise ValueError( - f"filter '{name}' used in an unsupported form (got '| {display}'): " - f"{_EXPECTED_FILTERS}" - ) + raise ValueError( + f"{name}: expected '| {name}' with no arguments or trailing " + f"tokens, got '| {display}'" + ) + if not call_form_ok(spec, len(arg_nodes), has_parens): + raise ValueError( + f"filter '{name}' used in an unsupported form (got '| {display}'): " + f"{_expected_filters()}" + ) + evaluated_args = [_eval(arg, namespace_) for arg in arg_nodes] + return spec.function(value, *evaluated_args) def _eval(node: Any, namespace_: dict[str, Any]) -> Any: @@ -580,13 +577,7 @@ def _check_filter(filt: tuple[str, str, tuple[Any, ...], bool]) -> str | None: spec = FILTER_TABLE.get(name) if spec is None: return f"unknown filter '{name}'" - if not spec.argument: - if has_parens: - return f"{name}: expected '| {name}' with no arguments or trailing tokens" - return None - if has_parens and len(args) == 1: - return None - if not has_parens and spec.bare: + if call_form_ok(spec, len(args), has_parens): return None return f"filter '{name}' used in an unsupported form" diff --git a/src/specify_cli/workflows/expressions/filters.py b/src/specify_cli/workflows/expressions/filters.py index 47ece27430..141f0197f0 100644 --- a/src/specify_cli/workflows/expressions/filters.py +++ b/src/specify_cli/workflows/expressions/filters.py @@ -9,8 +9,9 @@ from __future__ import annotations import json -from collections.abc import Callable +from collections.abc import Callable, Mapping from dataclasses import dataclass +from types import MappingProxyType from typing import Any, NoReturn @@ -276,50 +277,117 @@ def _check_json_keys(value: Any) -> None: @dataclass(frozen=True) class FilterSpec: - """One pipe filter: its function and the call forms it accepts. - - ``argument``: the filter takes exactly one parenthesised argument - (``join('sep')``). ``bare``: the unparenthesised form is also valid - (``default`` alone). Zero-argument filters (``from_json``, ``upper``, ...) - have ``argument=False`` and are only valid bare — the strictness that turns - ``from_json()``, ``from_json)`` and ``from_json extra`` into errors naming - the filter instead of a silent passthrough. - """ + """One pipe filter and the number of explicit arguments it accepts.""" function: Callable[..., Any] - argument: bool - bare: bool = False + min_args: int + max_args: int + # Built-in diagnostics historically used hand-written examples. Keep the + # override so the arity refactor does not alter established messages. usage: str = "" +def format_filter_usage(name: str, min_args: int, max_args: int) -> str: + """Return generic usage text for a filter's argument bounds.""" + if min_args == max_args == 0: + return name + if min_args == 0 and max_args == 1: + return f"{name} or {name}('x')" + if min_args == max_args == 1: + return f"{name}('arg1')" + args = ", ".join(f"arg{i}" for i in range(1, max_args + 1)) + call = f"{name}({args})" + return f"{name} or {call}" if min_args == 0 else call + + +def call_form_ok(spec: FilterSpec, arg_count: int, has_parens: bool) -> bool: + """Whether a call with *arg_count* arguments is valid for *spec*. + + The bare form (``| name``) is valid when no argument is required. A + parenthesized call needs at least one argument, within the filter's bounds, + so ``| name()`` is never valid. Shared by evaluation and static validation + so the two cannot disagree. + """ + if has_parens: + return max(1, spec.min_args) <= arg_count <= spec.max_args + return spec.min_args == 0 + + FILTER_TABLE: dict[str, FilterSpec] = { - "default": FilterSpec( - _filter_default, argument=True, bare=True, usage="default or default('x')" - ), - "join": FilterSpec(_filter_join, argument=True, usage="join('sep')"), - "map": FilterSpec(_filter_map, argument=True, usage="map('attr')"), - "contains": FilterSpec(_filter_contains, argument=True, usage="contains('s')"), - "split": FilterSpec(_filter_split, argument=True, usage="split('sep')"), - "from_json": FilterSpec(_filter_from_json, argument=False, usage="from_json"), - "upper": FilterSpec(_filter_upper, argument=False, usage="upper"), - "lower": FilterSpec(_filter_lower, argument=False, usage="lower"), - "length": FilterSpec(_filter_length, argument=False, usage="length"), - "to_json": FilterSpec(_filter_to_json, argument=False, usage="to_json"), + "default": FilterSpec(_filter_default, 0, 1, "default or default('x')"), + "join": FilterSpec(_filter_join, 1, 1, "join('sep')"), + "map": FilterSpec(_filter_map, 1, 1, "map('attr')"), + "contains": FilterSpec(_filter_contains, 1, 1, "contains('s')"), + "split": FilterSpec(_filter_split, 1, 1, "split('sep')"), + "from_json": FilterSpec(_filter_from_json, 0, 0, "from_json"), + "upper": FilterSpec(_filter_upper, 0, 0, "upper"), + "lower": FilterSpec(_filter_lower, 0, 0, "lower"), + "length": FilterSpec(_filter_length, 0, 0, "length"), + "to_json": FilterSpec(_filter_to_json, 0, 0, "to_json"), } +BUILTIN_FILTERS: Mapping[str, FilterSpec] = MappingProxyType(dict(FILTER_TABLE)) +_REGISTERED_FILTERS: tuple[str, ...] = tuple(BUILTIN_FILTERS) + + +def _expected_filters() -> str: + """Build the expected-filter diagnostic from the live registry.""" + usages = [ + spec.usage or format_filter_usage(name, spec.min_args, spec.max_args) + for name, spec in FILTER_TABLE.items() + ] + if not usages: + return "expected one of no filters" + if len(usages) == 1: + return f"expected one of {usages[0]}" + return "expected one of " + ", ".join(usages[:-1]) + ", or " + usages[-1] + + +def register_custom_filter( + name: str, function: Callable[..., Any], min_args: int = 0, max_args: int = 0 +) -> None: + """Register a project function in the process-local filter table.""" + import re + + if not isinstance(name, str) or re.fullmatch(r"[a-z_][a-z0-9_]*", name) is None: + raise ValueError( + f"Invalid custom filter name {name!r}: expected a lowercase identifier" + ) + if name in BUILTIN_FILTERS: + raise KeyError(f"Filter '{name}' is a built-in filter") + if name in FILTER_TABLE: + raise KeyError(f"Filter '{name}' is already registered") + if not callable(function): + raise TypeError(f"Custom filter '{name}' handler must be callable") + if ( + isinstance(min_args, bool) or isinstance(max_args, bool) + or not isinstance(min_args, int) or not isinstance(max_args, int) + or min_args < 0 or max_args < 0 or min_args > max_args + ): + raise ValueError( + f"Invalid argument bounds for custom filter '{name}': expected " + "non-negative integers with min_args <= max_args" + ) + + def normalized(value: Any, *args: Any) -> Any: + try: + return function(value, *args) + except Exception as exc: # noqa: BLE001 - custom code boundary + raise ValueError(f"{name}: {exc}") from exc + + FILTER_TABLE[name] = FilterSpec( + normalized, min_args, max_args, + format_filter_usage(name, min_args, max_args), + ) + -# The filters the expression evaluator recognizes. Used to tell a -# *registered* filter used in an unsupported form (e.g. `| join` with no -# argument) apart from a genuinely unknown filter name, so each raises an -# error that names the real problem. -_REGISTERED_FILTERS: tuple[str, ...] = tuple(FILTER_TABLE) +def unregister_custom_filter(name: str) -> None: + """Remove one project filter when a package registration is rolled back.""" + if name not in BUILTIN_FILTERS: + FILTER_TABLE.pop(name, None) -# The sentence every filter-wiring error ends with, derived from the table so -# adding a filter cannot leave the message listing the old set. -_EXPECTED_FILTERS = ( - "expected one of " - + ", ".join(spec.usage for spec in list(FILTER_TABLE.values())[:-1]) - + ", or " - + list(FILTER_TABLE.values())[-1].usage -) +def unregister_custom_filters() -> None: + """Restore the built-in filter table.""" + for name in tuple(FILTER_TABLE): + unregister_custom_filter(name) diff --git a/tests/specify_cli/workflows/function/__init__.py b/tests/specify_cli/workflows/function/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/specify_cli/workflows/function/conftest.py b/tests/specify_cli/workflows/function/conftest.py new file mode 100644 index 0000000000..7bbbe9182e --- /dev/null +++ b/tests/specify_cli/workflows/function/conftest.py @@ -0,0 +1,18 @@ +"""Fixtures for workflow function tests. + +The custom-filter table is process-global, so every test starts and ends with +only built-in filters registered. +""" + +from __future__ import annotations + +import pytest + + +@pytest.fixture(autouse=True) +def _reset_custom_filters(): + from specify_cli.workflows.expressions import unregister_custom_filters + + unregister_custom_filters() + yield + unregister_custom_filters() diff --git a/tests/specify_cli/workflows/function/test_custom_filters.py b/tests/specify_cli/workflows/function/test_custom_filters.py new file mode 100644 index 0000000000..bf7a1f4f22 --- /dev/null +++ b/tests/specify_cli/workflows/function/test_custom_filters.py @@ -0,0 +1,385 @@ +"""Filter table: arity, registration, evaluation and handler adaptation.""" + +from __future__ import annotations + +import pytest + +from specify_cli.workflows.base import StepContext +from specify_cli.workflows.expressions import ( + BUILTIN_FILTERS, + FILTER_TABLE, + _REGISTERED_FILTERS, + _evaluator_rejects, + evaluate_condition, + evaluate_expression, + format_filter_usage, + register_custom_filter, + unregister_custom_filter, + unregister_custom_filters, +) + +BUILTIN_NAMES = ( + "default", "join", "map", "contains", "split", + "from_json", "upper", "lower", "length", "to_json", +) +BUILTIN_LIST = ( + "default or default('x'), join('sep'), map('attr'), contains('s'), " + "split('sep'), from_json, upper, lower, length, or to_json" +) + + +def ctx(**inputs): + return StepContext(inputs=inputs) + + +def reg(name, function=lambda value: value, minimum=0, maximum=0): + register_custom_filter(name, function, minimum, maximum) + + +# -- Arity refactor: built-in behaviour and messages are unchanged ----------- + + +class TestBuiltinFiltersUnchanged: + def test_builtin_snapshot_and_registered_names(self): + assert tuple(BUILTIN_FILTERS) == BUILTIN_NAMES + assert _REGISTERED_FILTERS == BUILTIN_NAMES + assert {n: (s.min_args, s.max_args) for n, s in BUILTIN_FILTERS.items()} == { + "default": (0, 1), "join": (1, 1), "map": (1, 1), "contains": (1, 1), + "split": (1, 1), "from_json": (0, 0), "upper": (0, 0), + "lower": (0, 0), "length": (0, 0), "to_json": (0, 0), + } + + def test_builtin_snapshot_is_read_only(self): + with pytest.raises(TypeError): + BUILTIN_FILTERS["x"] = BUILTIN_FILTERS["upper"] # type: ignore[index] + + @pytest.mark.parametrize( + ("template", "message"), + [ + ( + "{{ inputs.s | bogus }}", + f"unknown filter 'bogus': expected one of {BUILTIN_LIST} (got '| bogus')", + ), + ( + "{{ inputs.s | join }}", + "filter 'join' used in an unsupported form (got '| join'): " + f"expected one of {BUILTIN_LIST}", + ), + ( + "{{ inputs.s | default() }}", + "filter 'default' used in an unsupported form " + f"(got '| default(...)'): expected one of {BUILTIN_LIST}", + ), + ( + "{{ inputs.s | upper() }}", + "upper: expected '| upper' with no arguments or trailing tokens, " + "got '| upper(...)'", + ), + ], + ) + def test_builtin_messages_are_byte_identical(self, template, message): + with pytest.raises(ValueError) as info: + evaluate_expression(template, ctx(s="x")) + assert str(info.value) == message + + def test_builtins_still_evaluate(self): + context = ctx(s="ab", missing=None, items=["a", "b"]) + assert evaluate_expression("{{ inputs.s | upper }}", context) == "AB" + assert evaluate_expression("{{ inputs.items | join('-') }}", context) == "a-b" + assert evaluate_expression("{{ inputs.nope | default }}", context) in (None, "") + assert evaluate_expression("{{ inputs.nope | default('z') }}", context) == "z" + + +# -- Usage formatter ---------------------------------------------------------- + + +class TestUsageFormatter: + @pytest.mark.parametrize( + ("minimum", "maximum", "expected"), + [ + (0, 0, "f"), + (0, 1, "f or f('x')"), + (1, 1, "f('arg1')"), + (1, 2, "f(arg1, arg2)"), + (2, 3, "f(arg1, arg2, arg3)"), + (0, 2, "f or f(arg1, arg2)"), + ], + ) + def test_generic_placeholders(self, minimum, maximum, expected): + assert format_filter_usage("f", minimum, maximum) == expected + + def test_custom_usage_is_listed_in_diagnostics(self): + reg("pad", lambda v, a, b="": v, 2, 3) + reg("trim") + with pytest.raises(ValueError) as info: + evaluate_expression("{{ inputs.s | nope }}", ctx(s="x")) + message = str(info.value) + assert "pad(arg1, arg2, arg3)" in message + assert message.endswith("or trim (got '| nope')") + + def test_builtin_usage_overrides_stay_hand_written(self): + assert BUILTIN_FILTERS["map"].usage == "map('attr')" + assert BUILTIN_FILTERS["default"].usage == "default or default('x')" + + +# -- Registration API --------------------------------------------------------- + + +class TestRegistration: + def test_register_adds_to_live_table_only(self): + reg("trim") + assert "trim" in FILTER_TABLE + assert "trim" not in BUILTIN_FILTERS + assert "trim" not in _REGISTERED_FILTERS + + @pytest.mark.parametrize("name", BUILTIN_NAMES) + def test_builtin_name_is_rejected_and_unchanged(self, name): + before = FILTER_TABLE[name] + with pytest.raises(KeyError, match="built-in"): + reg(name) + assert FILTER_TABLE[name] is before + + def test_duplicate_is_rejected(self): + reg("trim") + with pytest.raises(KeyError, match="already registered"): + reg("trim") + + @pytest.mark.parametrize("name", ["Trim", "a-b", "1x", "", "a b", "a.b", 5, None]) + def test_invalid_names_are_rejected(self, name): + with pytest.raises(ValueError, match="Invalid custom filter name"): + reg(name) + assert name not in FILTER_TABLE + + def test_non_callable_handler_is_rejected(self): + with pytest.raises(TypeError, match="callable"): + register_custom_filter("x", "not callable") # type: ignore[arg-type] + + @pytest.mark.parametrize( + ("minimum", "maximum"), + [(2, 1), (-1, 1), (0, -1), (True, 1), (0, True), (0.0, 1), (0, "1")], + ) + def test_invalid_bounds_are_rejected(self, minimum, maximum): + with pytest.raises(ValueError, match="argument bounds"): + reg("x", minimum=minimum, maximum=maximum) + assert "x" not in FILTER_TABLE + + def test_unregister_removes_only_custom_filters(self): + reg("a") + reg("b") + unregister_custom_filter("a") + unregister_custom_filter("upper") + assert "a" not in FILTER_TABLE and "b" in FILTER_TABLE + assert "upper" in FILTER_TABLE + unregister_custom_filters() + assert tuple(FILTER_TABLE) == BUILTIN_NAMES + + +# -- Evaluation --------------------------------------------------------------- + + +class TestEvaluation: + def test_interpolation_in_text(self): + reg("trim", lambda v: v.strip()) + assert evaluate_expression("<{{ inputs.s | trim }}>", ctx(s=" x ")) == "" + + def test_single_expression_keeps_the_returned_type(self): + reg("pair", lambda v: [v, v]) + assert evaluate_expression("{{ inputs.s | pair }}", ctx(s="x")) == ["x", "x"] + + def test_chains_with_builtins_in_both_directions(self): + reg("trim", lambda v: v.strip()) + assert evaluate_expression("{{ inputs.s | trim | upper }}", ctx(s=" ab ")) == "AB" + assert evaluate_expression("{{ inputs.s | upper | trim }}", ctx(s=" ab ")) == "AB" + + def test_zero_args(self): + reg("trim", lambda v: v.strip()) + assert evaluate_expression("{{ inputs.s | trim }}", ctx(s=" a ")) == "a" + + def test_optional_single_argument(self): + reg("suffix", lambda v, s="!": v + s, 0, 1) + context = ctx(s="a") + assert evaluate_expression("{{ inputs.s | suffix }}", context) == "a!" + assert evaluate_expression("{{ inputs.s | suffix('?') }}", context) == "a?" + + def test_required_single_argument(self): + reg("wrap", lambda v, q: f"{q}{v}{q}", 1, 1) + assert evaluate_expression("{{ inputs.s | wrap('\"') }}", ctx(s="a")) == '"a"' + + def test_ranged_arguments_are_passed_positionally_after_the_value(self): + reg("pad", lambda v, left, right, extra="": f"{left}{v}{right}{extra}", 2, 3) + context = ctx(s="x") + with pytest.raises(ValueError): + evaluate_expression("{{ inputs.s | pad('[') }}", context) + assert evaluate_expression("{{ inputs.s | pad('[', ']') }}", context) == "[x]" + assert evaluate_expression("{{ inputs.s | pad('[', ']', '!') }}", context) == "[x]!" + + def test_arguments_may_be_expressions(self): + reg("wrap", lambda v, q: f"{q}{v}{q}", 1, 1) + assert evaluate_expression("{{ inputs.s | wrap(inputs.q) }}", ctx(s="a", q="|")) == "|a|" + + def test_condition_uses_custom_filter(self): + reg("blank", lambda v: not v.strip()) + assert evaluate_condition("{{ inputs.s | blank }}", ctx(s=" ")) is True + assert evaluate_condition("{{ inputs.s | blank }}", ctx(s="x")) is False + + @pytest.mark.parametrize( + ("minimum", "maximum", "template"), + [ + (2, 3, "{{ inputs.s | f }}"), + (2, 3, "{{ inputs.s | f('a') }}"), + (2, 3, "{{ inputs.s | f('a', 'b', 'c', 'd') }}"), + (1, 1, "{{ inputs.s | f }}"), + (1, 1, "{{ inputs.s | f('a', 'b') }}"), + (0, 1, "{{ inputs.s | f() }}"), + (0, 1, "{{ inputs.s | f('a', 'b') }}"), + ], + ) + def test_wrong_call_form_names_the_filter(self, minimum, maximum, template): + reg("f", lambda v, *a: v, minimum, maximum) + with pytest.raises(ValueError, match="filter 'f' used in an unsupported form"): + evaluate_expression(template, ctx(s="x")) + + def test_parentheses_on_zero_arg_filter_are_rejected(self): + reg("f") + with pytest.raises(ValueError, match=r"f: expected '\| f' with no arguments"): + evaluate_expression("{{ inputs.s | f() }}", ctx(s="x")) + + def test_unregistered_name_is_unknown_after_unload(self): + reg("f") + unregister_custom_filters() + with pytest.raises(ValueError, match="unknown filter 'f'"): + evaluate_expression("{{ inputs.s | f }}", ctx(s="x")) + + +# -- Validation parity (same set of filters as evaluation) ------------------- + + +class TestValidationParity: + def test_evaluator_check_accepts_registered_and_rejects_unknown(self): + assert _evaluator_rejects("inputs.s | f") == "unknown filter 'f'" + reg("f", lambda v, a: v, 1, 1) + assert _evaluator_rejects("inputs.s | f('a')") is None + assert _evaluator_rejects("inputs.s | f") == "filter 'f' used in an unsupported form" + assert _evaluator_rejects("inputs.s | f('a', 'b')") == ( + "filter 'f' used in an unsupported form" + ) + + def test_check_and_evaluation_agree_on_every_call_form(self): + reg("f", lambda v, *a: v, 1, 2) + for form in ("f", "f()", "f('a')", "f('a', 'b')", "f('a', 'b', 'c')"): + rejected = _evaluator_rejects(f"inputs.s | {form}") is not None + try: + evaluate_expression(f"{{{{ inputs.s | {form} }}}}", ctx(s="x")) + raised = False + except ValueError: + raised = True + assert rejected == raised, form + + def test_validate_workflow_reports_unknown_filter_then_accepts_loaded(self): + from specify_cli.workflows.engine import WorkflowDefinition, validate_workflow + + definition = WorkflowDefinition.from_string( + """ +schema_version: "1.0" +workflow: {id: w, name: W, version: "1.0.0"} +steps: + - id: a + type: if + condition: "inputs.s | blank" + then: + - id: b + type: shell + run: "echo hi" +""" + ) + errors = validate_workflow(definition) + assert len(errors) == 1 and "unknown filter 'blank'" in errors[0] + + reg("blank", lambda v: not v) + errors = validate_workflow(definition) + assert len(errors) == 1 + assert "unknown filter" not in errors[0] + assert '"{{ inputs.s | blank }}"' in errors[0] + + +# -- Handler adapter (D7) ------------------------------------------------------ + + +class TestHandlerAdapter: + def test_arguments_and_return_value_pass_through_untouched(self): + seen = [] + marker = object() + + def handler(value, *args): + seen.append((value, args)) + return marker + + reg("f", handler, 0, 2) + assert evaluate_expression("{{ inputs.s | f('a', 'b') }}", ctx(s="x")) is marker + assert seen == [("x", ("a", "b"))] + + def test_ordinary_exception_becomes_value_error_with_cause(self): + def handler(value): + raise TypeError("needs a string") + + reg("boom", handler) + with pytest.raises(ValueError) as info: + evaluate_expression("{{ inputs.s | boom }}", ctx(s="x")) + assert str(info.value) == "boom: needs a string" + assert isinstance(info.value.__cause__, TypeError) + + def test_value_error_is_also_prefixed(self): + def handler(value): + raise ValueError("bad input") + + reg("boom", handler) + with pytest.raises(ValueError, match="^boom: bad input$"): + evaluate_expression("{{ inputs.s | boom }}", ctx(s="x")) + + @pytest.mark.parametrize("exception", [SystemExit, KeyboardInterrupt, GeneratorExit]) + def test_base_exceptions_pass_through(self, exception): + def handler(value): + raise exception() + + reg("boom", handler) + with pytest.raises(exception): + evaluate_expression("{{ inputs.s | boom }}", ctx(s="x")) + + def test_builtin_errors_are_not_rewrapped(self): + reg("noop") + with pytest.raises(ValueError) as info: + evaluate_expression("{{ inputs.n | upper }}", ctx(n=3)) + assert not str(info.value).startswith("noop") + assert str(info.value).startswith("upper") + + +# -- Return values (D8): no function-specific validation ---------------------- + + +class TestReturnValues: + @pytest.mark.parametrize( + "value", [None, 0, False, "", ["a"], {"k": [1, 2]}, 1.5, b"bytes", {1, 2}] + ) + def test_any_value_reaches_a_typed_template(self, value): + reg("make", lambda v: value) + result = evaluate_expression("{{ inputs.s | make }}", ctx(s="x")) + assert result == value and type(result) is type(value) + + def test_returned_object_is_not_inspected_or_copied(self): + class Opaque: + pass + + instance = Opaque() + reg("make", lambda v: instance) + assert evaluate_expression("{{ inputs.s | make }}", ctx(s="x")) is instance + + def test_result_feeds_a_builtin_with_its_own_validation(self): + reg("make_int", lambda v: 3) + with pytest.raises(ValueError, match="upper"): + evaluate_expression("{{ inputs.s | make_int | upper }}", ctx(s="x")) + + def test_result_round_trips_through_to_json(self): + reg("make", lambda v: {"a": [1]}) + assert evaluate_expression("{{ inputs.s | make | to_json }}", ctx(s="x")) == ( + '{"a": [1]}' + ) From 3546fc9f06b09fa977bf3d4e17c2dc808fbe24b1 Mon Sep 17 00:00:00 2001 From: Markus Date: Sat, 7 Nov 2026 08:39:24 +0100 Subject: [PATCH 02/11] refactor(workflows): share package install, registry, URL and removal mechanics Move the kind-independent parts of step package handling into reusable modules so another installed package kind does not need a copy: - _package_install: path/symlink safety, no-follow tree copy with excludes and budgets, the project lock, publish-with-rollback, removal, and one staged install_package flow, parameterized by InstallRequest, PackageSpec, PackageRules and StagingOps value objects. - _package_registry: PackageRegistry (atomic, symlink-safe JSON registry with in-memory rollback); StepRegistry now only names its file and key. - _archive_install: the --from URL flow (trust prompt, download, archive format agreement, safe extract, cleanup) and a shared error renderer. - workflows/__init__: extract _import_package/_drop_modules from load_custom_steps. The step installer keeps every established name as a thin binding and builds its spec at call time, so existing patch points, messages and the standalone loading used by the community step validator are unchanged. Existing step tests are not modified. Refs #5285 Assisted-by: pi coding agent (GitHub Copilot, model: claude-sonnet-5.5, autonomous) --- src/specify_cli/workflows/__init__.py | 64 +- src/specify_cli/workflows/_archive_install.py | 233 +++++ src/specify_cli/workflows/_commands.py | 7 + src/specify_cli/workflows/_package_install.py | 887 ++++++++++++++++++ .../workflows/_package_registry.py | 199 ++++ .../workflows/step/catalog/_domain.py | 174 +--- src/specify_cli/workflows/step/command_add.py | 214 +---- .../workflows/step/command_remove.py | 131 +-- src/specify_cli/workflows/step/installer.py | 643 +++---------- .../step/test_installer_standalone.py | 48 + 10 files changed, 1601 insertions(+), 999 deletions(-) create mode 100644 src/specify_cli/workflows/_archive_install.py create mode 100644 src/specify_cli/workflows/_package_install.py create mode 100644 src/specify_cli/workflows/_package_registry.py create mode 100644 tests/specify_cli/workflows/step/test_installer_standalone.py diff --git a/src/specify_cli/workflows/__init__.py b/src/specify_cli/workflows/__init__.py index 2f457371ad..094d54a52f 100644 --- a/src/specify_cli/workflows/__init__.py +++ b/src/specify_cli/workflows/__init__.py @@ -87,18 +87,47 @@ def _register_builtin_steps() -> None: _CUSTOM_STEP_MODULES: set[str] = set() -def _unload_custom_steps() -> None: - """Clear custom registrations and synthetic imports from a prior project.""" +def _import_package(module_name: str, package_dir: Path): + """Import an installed package under an isolated synthetic module name.""" + import importlib.util import sys - for type_key in tuple(STEP_REGISTRY): - if type_key not in BUILTIN_STEP_TYPES: - del STEP_REGISTRY[type_key] - for module_name in _CUSTOM_STEP_MODULES: + init_py = Path(package_dir) / "__init__.py" + spec = importlib.util.spec_from_file_location( + module_name, + init_py, + submodule_search_locations=[str(package_dir)], + ) + if spec is None or spec.loader is None: + raise ImportError(f"Could not create an import specification for '{package_dir}'") + module = importlib.util.module_from_spec(spec) + module.__package__ = module_name + sys.modules[module_name] = module + try: + spec.loader.exec_module(module) # type: ignore[union-attr] + except BaseException: + _drop_modules({module_name}) + raise + return module + + +def _drop_modules(module_names: set[str] | list[str] | tuple[str, ...]) -> None: + """Remove synthetic modules and their relative-import children.""" + import sys + + for module_name in module_names: sys.modules.pop(module_name, None) prefix = module_name + "." for loaded_name in [name for name in sys.modules if name.startswith(prefix)]: sys.modules.pop(loaded_name, None) + + +def _unload_custom_steps() -> None: + """Clear custom registrations and synthetic imports from a prior project.""" + for type_key in tuple(STEP_REGISTRY): + if type_key not in BUILTIN_STEP_TYPES: + del STEP_REGISTRY[type_key] + _drop_modules(_CUSTOM_STEP_MODULES) _CUSTOM_STEP_MODULES.clear() @@ -113,9 +142,7 @@ def load_custom_steps(project_root: Path) -> list[str]: Silently skips packages that fail to import or validate. """ import hashlib as _hashlib - import importlib.util as _importlib_util import re as _re - import sys as _sys _unload_custom_steps() steps_dir = Path(project_root) / ".specify" / "workflows" / "steps" @@ -174,21 +201,9 @@ def load_custom_steps(project_root: Path) -> list[str]: # Treat the step directory as a proper package so that relative # imports inside the step (e.g. ``from .helpers import …``) work. - spec = _importlib_util.spec_from_file_location( - module_name, - init_py, - submodule_search_locations=[str(step_dir)], - ) - if spec is None or spec.loader is None: - continue - module = _importlib_util.module_from_spec(spec) - module.__package__ = module_name - # Register before exec so relative imports resolve correctly. - _sys.modules[module_name] = module + module = _import_package(module_name, step_dir) registered = False try: - spec.loader.exec_module(module) # type: ignore[union-attr] - # Find the StepBase subclass in the module from .base import StepBase as _StepBase @@ -222,12 +237,7 @@ def load_custom_steps(project_root: Path) -> list[str]: # a broken/skipped step package leaves no lingering import state # behind. if not registered: - _sys.modules.pop(module_name, None) - submodule_prefix = module_name + "." - for _mod_key in [ - k for k in _sys.modules if k.startswith(submodule_prefix) - ]: - _sys.modules.pop(_mod_key, None) + _drop_modules({module_name}) except Exception: # noqa: BLE001, S112 # Silently skip broken step packages at load time continue diff --git a/src/specify_cli/workflows/_archive_install.py b/src/specify_cli/workflows/_archive_install.py new file mode 100644 index 0000000000..0e03987743 --- /dev/null +++ b/src/specify_cli/workflows/_archive_install.py @@ -0,0 +1,233 @@ +"""CLI flow shared by ``workflow step add --from`` and ``workflow function add --from``. + +Downloading an archive from a URL, confirming trust, extracting it safely and +handing the package root to the kind's installer is identical for every package +kind that contains executable Python. The kind-specific parts are supplied as +:class:`ArchiveHooks`; naming and the error type come from the shared +:class:`~specify_cli.workflows._package_install.PackageSpec`. + +This module is a CLI layer (it prompts and prints); the domain installers it +calls stay CLI-independent. +""" + +from __future__ import annotations + +import sys +from collections.abc import Callable +from dataclasses import dataclass +from pathlib import Path +from typing import Any + +from . import _commands as cli +from ._package_install import PackageSpec + + +@dataclass(frozen=True) +class ArchiveRequest: + """What to install from which URL.""" + + project_root: Path + package_id: str + url: str + force: bool = False + + +@dataclass(frozen=True) +class ArchiveHooks: + """Package-kind steps of the URL flow.""" + + preflight: Callable[[Path, str, bool], Any] # (root, id, force) -> None; rejects early + resolve_root: Callable[[Path], Path] # extracted dir -> package dir + install: Callable[[Path, str, Path, bool], dict[str, Any]] # (root, id, pkg, force) + + +def _cleanup_download_tmp_path(tmp_path: Path | None, spec: PackageSpec) -> None: + """Best-effort unlink of a partially-downloaded archive temp file. + + A cleanup ``OSError`` must never replace/mask whatever error or interrupt is + already propagating -- warn about it and keep going. + """ + if tmp_path is None: + return + try: + tmp_path.unlink(missing_ok=True) + except OSError as cleanup_exc: + cli.console.print( + "[yellow]Warning:[/yellow] Could not remove temporary " + f"{spec.lower} download file: {cli._escape_markup(str(cleanup_exc))} " + f"(path: {cli._escape_markup(str(tmp_path))})" + ) + + +def install_from_archive_url( + request: ArchiveRequest, spec: PackageSpec, hooks: ArchiveHooks +) -> dict[str, Any]: + """Install a package archive from a direct URL and return its registry entry.""" + import tempfile + from urllib.parse import urlparse + + from rich.panel import Panel + + from specify_cli.authentication.github_http import ( + resolve_github_release_asset_api_url as _resolve_gh_asset, + ) + from specify_cli.authentication.http import ( + github_provider_hosts as _github_provider_hosts, + ) + from specify_cli.authentication.http import open_url as _open_url + + project_root, package_id, from_url, force = ( + request.project_root, request.package_id, request.url, request.force, + ) + error = spec.error + + try: + parsed = urlparse(from_url) + hostname = parsed.hostname + _ = parsed.port + except ValueError: + raise error(f"Invalid URL: {from_url}") from None + if not hostname: + raise error(f"Invalid URL: {from_url}") + if not cli.is_https_or_localhost_http(from_url): + raise error( + "URL must use HTTPS for security. HTTP is only allowed for " + "loopback URLs." + ) + + # Reject before the trust prompt and before any network request. + hooks.preflight(project_root, package_id, force) + + # Prompt BEFORE any request (and before any spinner) so the user can see + # and answer it; a declined prompt issues no request and exits 0. + kind = spec.kind_label.lower() + cli.console.print() + cli.console.print( + Panel( + f"[bold]You are installing a workflow {kind} directly from an " + f"external URL.\nA {spec.lower} package contains executable Python.[/bold]\n\n" + f"URL: {cli._escape_markup(from_url)}\n\n" + f"Only install {spec.lower} packages from sources you trust.", + title="[bold yellow]⚠ Untrusted Source[/bold yellow]", + border_style="yellow", + padding=(1, 2), + ) + ) + cli.console.print() + if not cli.typer.confirm("Continue with installation?", default=False): + cli.console.print("Cancelled") + raise cli.typer.Exit(0) + + download_url = from_url + extra_headers = None + tmp_path: Path | None = None + extract_tmp: tempfile.TemporaryDirectory[str] | None = None + committed = False + try: + resolved_url = _resolve_gh_asset( + from_url, + _open_url, + timeout=30, + github_hosts=_github_provider_hosts(), + redirect_validator=cli._reject_insecure_download_redirect, + ) + if resolved_url: + download_url = resolved_url + extra_headers = {"Accept": "application/octet-stream"} + + with _open_url( + download_url, + timeout=30, + extra_headers=extra_headers, + redirect_validator=cli._reject_insecure_download_redirect, + ) as resp: + final_url = resp.geturl() + if not cli.is_https_or_localhost_http(final_url): + raise error(f"URL redirected to non-HTTPS: {final_url}") + content_type = ( + resp.getheader("Content-Type") + if hasattr(resp, "getheader") + else None + ) + declarations = [ + ("requested URL", from_url, cli.archive_format_from_name(from_url)), + ("final URL", final_url, cli.archive_format_from_name(final_url)), + ( + "Content-Type", + content_type or "", + cli.archive_format_from_content_type(content_type), + ), + ] + recognized = [item for item in declarations if item[2] is not None] + archive_format = recognized[0][2] if recognized else None + if any(item[2] != archive_format for item in recognized): + details = ", ".join( + f"{label} declares {declared}" + for label, _value, declared in recognized + ) + raise error(f"Archive format mismatch: {details}") + downloaded = cli.read_response_limited( + resp, + error_type=ValueError, + label=f"{spec.lower} archive download", + ) + + with tempfile.NamedTemporaryFile( + suffix=cli.archive_suffix(archive_format) if archive_format else ".archive", + delete=False, + ) as tmp: + tmp_path = Path(tmp.name) + tmp.write(downloaded) + + try: + extract_tmp = tempfile.TemporaryDirectory( + prefix=f"speckit-{spec.lower}-archive-" + ) + except OSError as exc: + raise error( + f"Failed to create temporary {spec.lower} archive directory: {exc}" + ) from exc + extracted_root = Path(extract_tmp.name) + try: + # safe_extract_archive re-detects and confirms the archive bytes. + cli.safe_extract_archive( + tmp_path, + extracted_root, + source_name=recognized[0][1] if recognized else None, + content_type=content_type, + ) + package_root = hooks.resolve_root(extracted_root) + entry = hooks.install(project_root, package_id, package_root, force) + committed = True + finally: + primary_error = sys.exc_info()[1] + try: + extract_tmp.cleanup() + except OSError as cleanup_exc: + if extract_tmp.name and Path(extract_tmp.name).exists(): + detail = ( + f"{cli._escape_markup(str(cleanup_exc))} " + f"(path: {cli._escape_markup(extract_tmp.name)})" + ) + cli.console.print( + "[yellow]Warning:[/yellow] Could not remove temporary " + f"{spec.lower} archive directory: {detail}" + ) + if primary_error is None and not committed: + raise error( + f"Failed to remove temporary {spec.lower} archive directory: " + f"{cleanup_exc}" + ) from cleanup_exc + # Do not raise from cleanup: the primary installation error + # (if any) is already propagating, and after commit the install + # has succeeded. The warning above reports the residual path. + except cli.typer.Exit: + raise + except error: + raise + except Exception as exc: + raise error(f"Failed to install {spec.lower} from URL: {exc}") from exc + finally: + _cleanup_download_tmp_path(tmp_path, spec) + + return entry diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index 28ed6e9957..18a315528e 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -50,6 +50,13 @@ def _error_console(json_output: bool): return err_console if json_output else console +def _print_package_error(exc: Exception) -> None: + """Render a package install/remove failure: retained notes as warnings, then the error.""" + for note in getattr(exc, "__notes__", ()): + console.print(f"[yellow]Warning:[/yellow] {_escape_markup(note)}") + console.print(f"[red]Error:[/red] {_escape_markup(str(exc))}") + + def _fail_integration_load(exc: Exception, *, json_output: bool, run_id: str | None = None): """Surface adapter failures before a workflow run state can be created.""" if json_output: diff --git a/src/specify_cli/workflows/_package_install.py b/src/specify_cli/workflows/_package_install.py new file mode 100644 index 0000000000..a7af98da76 --- /dev/null +++ b/src/specify_cli/workflows/_package_install.py @@ -0,0 +1,887 @@ +"""Shared install mechanics for project-installed workflow packages. + +Step packages and function packages are both directories of executable Python +plus a manifest, installed under ``.specify/workflows/s//`` and +recorded in a registry. This module owns what is identical for both: path and +symlink safety, package-tree walking and copying, the staged install flow, the +project lock, and replacement/rollback. + +What differs per package kind is described by three small value objects: + +* :class:`PackageSpec` -- static identity (naming, error type, size limits). +* :class:`PackageRules` -- the kind's semantics (where it lives, how a manifest + is validated, which collisions are rejected, what registry entry is stored). +* :class:`StagingOps` -- the filesystem mechanics; :meth:`StagingOps.standard` + supplies the shared implementations and callers override single members only + when they must (the step installer does so to keep its established patch + points). + +:func:`install_package` combines them with an :class:`InstallRequest`. The module +is CLI-independent: it never prints and never raises ``typer.Exit``; failures +use ``spec.error``. +""" + +from __future__ import annotations + +import contextlib +import functools +import os +import shutil +import stat +import sys +import tempfile +import warnings +from collections.abc import Callable, Iterator +from dataclasses import dataclass +from pathlib import Path +from typing import Any, ContextManager + +# Files/dirs never inspected, copied into, or counted as part of an installed +# package; excluded directories are pruned without being entered. Mirrors +# ``bundles/packager.py`` ``EXCLUDE_NAMES``. +EXCLUDE_NAMES: frozenset[str] = frozenset({".git", "__pycache__", ".DS_Store"}) + +_COPY_CHUNK_BYTES = 64 * 1024 + + +# --------------------------------------------------------------------------- +# Value objects +# --------------------------------------------------------------------------- + + +@dataclass(frozen=True) +class PackageLimits: + """Ceilings applied uniformly to catalog, local, and archive sources.""" + + max_files: int + max_bytes: int + max_depth: int + + +@dataclass(frozen=True) +class PackageSpec: + """Static identity of a package kind. + + ``noun`` is the capitalized kind name (``"Step"``, ``"Function"``) used to + build user-facing messages; ``kind_label`` names an installed item + (``"Step type"``, ``"Function package"``). Directory, lock and staging + names are derived from ``noun`` so they cannot drift apart. + """ + + noun: str + kind_label: str + error: type[Exception] + limits: PackageLimits + registry_errors: tuple[type[Exception], ...] = (OSError, TypeError, ValueError) + excludes: frozenset[str] = EXCLUDE_NAMES + + @property + def lower(self) -> str: + return self.noun.lower() + + @property + def directory(self) -> str: + return f"{self.lower}s" + + @property + def remove_command(self) -> str: + return f"specify workflow {self.lower} remove" + + @property + def work_prefix(self) -> str: + return f".speckit-{self.lower}-install-" + + @property + def lock_name(self) -> str: + return f".{self.lower}-install.lock" + + +@dataclass(frozen=True) +class PackageLocation: + """Where a package is installed: the kind's base directory and its slot.""" + + base_dir: Path + destination: Path + + +@dataclass(frozen=True) +class InstallRequest: + """What is being installed.""" + + project_root: Path + package_id: str + package_dir: Path + force: bool = False + + +@dataclass(frozen=True) +class PackageRules: + """Package-kind semantics supplied to :func:`install_package`. + + ``check_destination`` rejects by id and destination state (built-ins, + duplicates); ``check_contents`` rejects by manifest content (for example + claimed names) and is optional. + """ + + locate: Callable[[Path, str], PackageLocation] + open_registry: Callable[[Path], Any] + check_destination: Callable[..., None] # (registry, id, location, *, force) + validate: Callable[[Path, str], Any] # (package_dir, id) -> parsed manifest + build_entry: Callable[[Any], dict[str, Any]] # parsed manifest -> registry entry + check_contents: Callable[..., None] | None = None # (registry, id, manifest, *, force) + + +@dataclass(frozen=True) +class StagingOps: + """Filesystem mechanics of an install.""" + + copy: Callable[[Path, Path], None] + publish: Callable[..., None] # (destination, staged, registry, id, entry, *, force) + transaction: Callable[[Path], ContextManager[None]] + + @classmethod + def standard(cls, spec: PackageSpec) -> StagingOps: + return cls( + copy=functools.partial(copy_package_tree, spec=spec), + publish=functools.partial(replace_install, spec=spec), + transaction=functools.partial(install_transaction, spec=spec), + ) + + +@dataclass(frozen=True) +class RemoveRequest: + """What is being removed.""" + + project_root: Path + package_id: str + + +@dataclass(frozen=True) +class RemoveHooks: + """Package-kind steps of a removal; ``transaction`` defaults to the shared lock.""" + + open_registry: Callable[[Path], Any] + transaction: Callable[[Path], ContextManager[None]] | None = None + + +@dataclass(frozen=True) +class RemovalOutcome: + """Result of a removal; ``orphaned`` means the directory had no registry entry.""" + + destination: Path + orphaned: bool + + +# --------------------------------------------------------------------------- +# Lock, directories and destination safety +# --------------------------------------------------------------------------- + + +@contextlib.contextmanager +def install_transaction(project_root: Path, spec: PackageSpec) -> Iterator[None]: + """Serialize package directory and registry mutations (install and remove).""" + from ..shared_infra import _exclusive_project_lock + + # Only acquisition failures are reported as lock errors; exceptions raised + # by the caller's critical section propagate unchanged. The message is + # operation-neutral because both install and remove share this lock; callers + # that add their own context should report ``exc.__cause__`` instead. + with contextlib.ExitStack() as stack: + try: + stack.enter_context( + _exclusive_project_lock( + Path(project_root), spec.lock_name, context=spec.lower + ) + ) + except OSError as exc: + raise spec.error(f"Failed to acquire the {spec.lower} lock: {exc}") from exc + yield + + +def resolve_base_dir(project_root: Path, spec: PackageSpec) -> Path: + """Resolve ``.specify/workflows/s`` refusing symlinked parent dirs.""" + project_root = Path(project_root) + project_root_resolved = project_root.resolve() + base_unresolved = project_root / ".specify" / "workflows" / spec.directory + + current = project_root + for part in (".specify", "workflows", spec.directory): + current = current / part + if current.is_symlink(): + raise spec.error( + f"Refusing to use symlinked {spec.lower} directory '{current}'" + ) + if current.exists() and not current.is_dir(): + raise spec.error( + f"{spec.noun} directory path is not a directory: '{current}'" + ) + + base_dir = base_unresolved.resolve() + try: + base_dir.relative_to(project_root_resolved) + except ValueError: + raise spec.error( + f"{spec.noun} directory escapes project root: '{base_dir}'" + ) from None + return base_dir + + +def resolve_destination(base_dir: Path, package_id: str, spec: PackageSpec) -> Path: + """Return the canonical destination directory for ``package_id``.""" + destination = base_dir / package_id + try: + rel_parts = destination.relative_to(base_dir).parts + except ValueError: + raise spec.error(f"Invalid {spec.lower} id '{package_id}'") from None + if rel_parts != (package_id,): + raise spec.error(f"Invalid {spec.lower} id '{package_id}'") + return destination + + +def reject_unsafe_destination(destination: Path, spec: PackageSpec) -> None: + """Refuse a symlink (including dangling) or non-directory destination.""" + if destination.is_symlink(): + raise spec.error( + f"Refusing to install {spec.lower} through a symlinked path: " + f"'{destination}'" + ) + if destination.exists() and not destination.is_dir(): + raise spec.error( + f"{spec.noun} install path exists but is not a directory: '{destination}'" + ) + + +def reject_source_overlap(package_dir: Path, destination: Path, spec: PackageSpec) -> None: + """Reject a source that resolves to (or contains) the install destination. + + A ``--force`` replacement would otherwise delete the source before it can + be copied. + """ + try: + source_resolved = package_dir.resolve() + dest_resolved = destination.resolve() + except OSError as exc: + raise spec.error( + f"Failed to resolve {spec.lower} package path: {exc}" + ) from exc + if source_resolved == dest_resolved or dest_resolved.is_relative_to( + source_resolved + ): + raise spec.error( + f"{spec.noun} package source resolves to the install destination: " + f"'{package_dir}'" + ) + + +# --------------------------------------------------------------------------- +# Package shape + safety validation +# --------------------------------------------------------------------------- + + +def entry_limit_error(spec: PackageSpec) -> Exception: + return spec.error( + f"{spec.noun} package contains too many entries, exceeding the " + f"{spec.limits.max_files}-entry limit (files and directories combined)" + ) + + +def scan_retained_entries( + directory: Path, budget: int, spec: PackageSpec +) -> list[os.DirEntry]: + """Return *directory*'s retained children, sorted by name. + + Streams the listing and drops ``EXCLUDE_NAMES`` without inspecting them. + Raises the entry-limit error as soon as more than *budget* retained + entries are seen, so an oversized directory is never fully materialized + or sorted. ``OSError`` propagates to the caller. + """ + entries: list[os.DirEntry] = [] + with os.scandir(directory) as iterator: + for entry in iterator: + if entry.name in spec.excludes: + continue + if len(entries) >= budget: + raise entry_limit_error(spec) + entries.append(entry) + entries.sort(key=lambda entry: entry.name) + return entries + + +def walk_package_tree(package_dir: Path, spec: PackageSpec): + """Yield ``(path, is_dir)`` for every retained descendant of *package_dir*. + + Excluded names are pruned without being entered or inspected, matching + ``bundles/packager.py``; their contents are never staged, so they are not + part of the package. Every retained entry discovered counts against the + entry limit before its directory listing is sorted, which bounds traversal + work by the entry ceiling. Never follows a symlink. Raises ``spec.error`` on + any symlink, object that is neither a regular file nor a directory, + nesting deeper than the depth limit, or too many entries. Iterative so deep + trees cannot exhaust the Python recursion limit. + """ + limits = spec.limits + discovered = 0 + + def _children(current: Path, depth: int): + nonlocal discovered + try: + entries = scan_retained_entries( + current, limits.max_files - discovered, spec + ) + except OSError as exc: + raise spec.error( + f"Failed to read {spec.lower} package directory '{current}': {exc}" + ) from exc + discovered += len(entries) + return [(entry, depth) for entry in reversed(entries)] + + stack = _children(package_dir, 1) + while stack: + entry, depth = stack.pop() + try: + mode = entry.stat(follow_symlinks=False).st_mode + except OSError as exc: + raise spec.error( + f"Failed to inspect {spec.lower} package entry '{entry.path}': {exc}" + ) from exc + path = Path(entry.path) + if stat.S_ISLNK(mode): + raise spec.error(f"{spec.noun} package contains symlink: {path}") + if stat.S_ISDIR(mode): + if depth > limits.max_depth: + raise spec.error( + f"{spec.noun} package exceeds the {limits.max_depth}-level " + "directory depth limit" + ) + yield path, True + stack.extend(_children(path, depth + 1)) + elif stat.S_ISREG(mode): + yield path, False + else: + raise spec.error(f"{spec.noun} package contains unsupported file: {path}") + + +def check_package_shape( + package_dir: Path, spec: PackageSpec, required_files: tuple[str, ...] +) -> None: + """Validate a materialized package directory's shape, symlinks and limits. + + Raises ``spec.error`` on any shape, symlink, limit or path violation. Never + imports or executes package code; manifest content is the caller's concern. + """ + package_dir = Path(package_dir) + + if package_dir.is_symlink(): + raise spec.error( + f"Refusing to install from a symlinked package directory: '{package_dir}'" + ) + if not package_dir.is_dir(): + raise spec.error(f"{spec.noun} package directory not found: '{package_dir}'") + + for required in required_files: + required_path = package_dir / required + if required_path.is_symlink(): + raise spec.error( + f"{spec.noun} package '{required}' must be a regular file, not a symlink" + ) + if not required_path.is_file(): + raise spec.error( + f"{spec.noun} package is missing required file '{required}' at its root" + ) + + # The walk enforces the entry ceiling itself; only bytes are summed here. + retained_bytes = 0 + for path, is_dir in walk_package_tree(package_dir, spec): + if is_dir: + continue + try: + retained_bytes += path.lstat().st_size + except OSError as exc: + raise spec.error( + f"Failed to inspect {spec.lower} package file '{path}': {exc}" + ) from exc + if retained_bytes > spec.limits.max_bytes: + raise spec.error( + f"{spec.noun} package exceeds the {spec.limits.max_bytes}-byte total " + "size limit" + ) + + +def resolve_package_root( + extracted_root: Path, spec: PackageSpec, manifest_name: str +) -> Path: + """Resolve a root-level or single-nested package directory.""" + extracted_root = Path(extracted_root) + root_manifest = extracted_root / manifest_name + if root_manifest.is_file() and not root_manifest.is_symlink(): + return extracted_root + try: + entries = list(extracted_root.iterdir()) + except OSError as exc: + raise spec.error(f"Failed to inspect archive contents: {exc}") from exc + if len(entries) == 1: + candidate = entries[0] + candidate_manifest = candidate / manifest_name + if ( + candidate.is_dir() + and not candidate.is_symlink() + and candidate_manifest.is_file() + and not candidate_manifest.is_symlink() + ): + return candidate + raise spec.error( + f"archive must contain {manifest_name} at its root or in exactly one " + "top-level directory" + ) + + +# --------------------------------------------------------------------------- +# Collision / duplicate preflight +# --------------------------------------------------------------------------- + + +def check_duplicate( + registry: Any, + package_id: str, + destination: Path, + *, + force: bool, + spec: PackageSpec, +) -> None: + """Reject case-insensitive aliases and, without ``force``, existing installs.""" + folded_id = package_id.casefold() + try: + registered_ids = registry.list() + except (AttributeError, TypeError): + registered_ids = () + for registered_id in registered_ids: + if ( + isinstance(registered_id, str) + and registered_id != package_id + and registered_id.casefold() == folded_id + ): + raise spec.error( + f"{spec.noun} ID '{package_id}' collides case-insensitively with " + f"registered {spec.lower} ID '{registered_id}'; use the exact " + "registered ID with --force" + ) + + try: + destination_exists = destination.exists() + for existing_path in destination.parent.iterdir(): + existing_name = existing_path.name + if existing_name == package_id: + continue + aliases_destination = ( + destination_exists and existing_path.samefile(destination) + ) + if existing_name.casefold() != folded_id and not aliases_destination: + continue + raise spec.error( + f"{spec.noun} ID '{package_id}' collides case-insensitively or " + f"resolves to the same filesystem path as existing {spec.lower} " + f"directory '{existing_name}'" + ) + except FileNotFoundError: + pass + except OSError as exc: + raise spec.error( + f"Failed to inspect existing {spec.lower} directories: {exc}" + ) from exc + + if force: + return + if registry.is_installed(package_id): + raise spec.error( + f"{spec.kind_label} '{package_id}' is already installed. Remove it " + f"first with: {spec.remove_command} {package_id}" + ) + if destination.exists(): + raise spec.error( + f"{spec.noun} directory already exists at '{destination}'. Remove it " + f"manually or use: {spec.remove_command} {package_id}" + ) + + +# --------------------------------------------------------------------------- +# Staging + commit +# --------------------------------------------------------------------------- + + +def copy_package_tree( + source_dir: Path, + target_dir: Path, + spec: PackageSpec, + *, + copy_file: Callable[[str, Path, int, int], int] | None = None, +) -> None: + """Copy *source_dir* into *target_dir*, skipping excludes. + + Refuses to follow a symlink encountered mid-copy so a source swapped after + validation cannot smuggle external content into the staged package, and + re-enforces the depth, entry, and byte budgets. Entries count against the + entry budget as each listing is streamed, before it is sorted, so an + oversized source directory is rejected without being fully read. Iterative + so a deep tree cannot exhaust the Python recursion limit. + + ``copy_file(source, target, expected_mode, remaining_bytes)`` copies one + inspected regular file and returns the bytes written. + """ + limits = spec.limits + if copy_file is None: + copy_file = functools.partial(copy_regular_file, spec=spec) + + remaining_bytes = limits.max_bytes + discovered_entries = 0 + pending = [(source_dir, target_dir, 0)] + while pending: + current, destination, depth = pending.pop() + if depth > limits.max_depth: + raise spec.error( + f"{spec.noun} package exceeds the {limits.max_depth}-level " + "directory depth limit" + ) + try: + destination.mkdir(parents=True, exist_ok=True) + except OSError as exc: + raise spec.error(f"Failed to stage {spec.lower} package: {exc}") from exc + try: + entries = scan_retained_entries( + current, limits.max_files - discovered_entries, spec + ) + except OSError as exc: + raise spec.error(f"Failed to stage {spec.lower} package: {exc}") from exc + discovered_entries += len(entries) + for entry in entries: + try: + mode = entry.stat(follow_symlinks=False).st_mode + except OSError as exc: + raise spec.error( + f"Failed to stage {spec.lower} package: {exc}" + ) from exc + target = destination / entry.name + if stat.S_ISLNK(mode): + raise spec.error(f"{spec.noun} package contains symlink: {entry.path}") + if not (stat.S_ISDIR(mode) or stat.S_ISREG(mode)): + raise spec.error( + f"{spec.noun} package contains unsupported file: {entry.path}" + ) + if stat.S_ISDIR(mode): + pending.append((Path(entry.path), target, depth + 1)) + else: + remaining_bytes -= copy_file(entry.path, target, mode, remaining_bytes) + + +def copy_regular_file( + source: str, + target: Path, + expected_mode: int, + remaining_bytes: int, + spec: PackageSpec, +) -> int: + """Copy an inspected regular file within the remaining package byte budget.""" + flags = os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0) + try: + fd = os.open(source, flags) + except OSError as exc: + raise spec.error(f"Failed to stage {spec.lower} package: {exc}") from exc + try: + opened = os.fstat(fd) + source_state = os.stat(source, follow_symlinks=False) + if ( + not stat.S_ISREG(opened.st_mode) + or not stat.S_ISREG(source_state.st_mode) + or opened.st_dev != source_state.st_dev + or opened.st_ino != source_state.st_ino + or stat.S_IFMT(source_state.st_mode) != stat.S_IFMT(expected_mode) + ): + raise spec.error( + f"{spec.noun} package file changed while staging: {source}" + ) + copied_bytes = 0 + with os.fdopen(fd, "rb", closefd=False) as source_file, target.open( + "xb" + ) as target_file: + while True: + chunk = source_file.read( + min(_COPY_CHUNK_BYTES, remaining_bytes - copied_bytes + 1) + ) + if not chunk: + break + if copied_bytes + len(chunk) > remaining_bytes: + raise spec.error( + f"{spec.noun} package exceeds the {spec.limits.max_bytes}-byte " + "total size limit while staging" + ) + target_file.write(chunk) + copied_bytes += len(chunk) + return copied_bytes + except OSError as exc: + raise spec.error(f"Failed to stage {spec.lower} package: {exc}") from exc + finally: + try: + os.close(fd) + except OSError: + pass + + +def replace_install( + destination: Path, + staged_dir: Path, + registry: Any, + package_id: str, + entry: dict[str, Any], + *, + force: bool, + spec: PackageSpec, +) -> None: + """Publish the staged package and record its registry entry.""" + if destination.exists(): + if not force: + raise spec.error( + f"{spec.noun} directory already exists at '{destination}'. Remove it " + f"manually or use: {spec.remove_command} {package_id}" + ) + # --force replacement: the replacement is fully staged and validated, + # so it is safe to remove the previous installation now. + try: + shutil.rmtree(destination) + except OSError as exc: + raise spec.error( + f"Failed to remove the existing {spec.lower} installation at " + f"'{destination}': {exc}. Reinstall from the original source with " + "--force." + ) from exc + try: + os.replace(staged_dir, destination) + except OSError as exc: + raise spec.error( + f"Failed to publish the replacement for {spec.kind_label.lower()} " + f"'{package_id}': {exc}. The previous installation was removed; " + "reinstall from the original source with --force." + ) from exc + try: + registry.add(package_id, entry) + except spec.registry_errors as exc: + raise spec.error( + f"Failed to update the {spec.lower} registry for '{package_id}': " + f"{exc}. The package was replaced but the registry metadata was not " + "updated; reinstall from the original source with --force." + ) from exc + return + + try: + os.replace(staged_dir, destination) + except OSError as exc: + raise spec.error( + f"Failed to install {spec.lower} '{package_id}': {exc}" + ) from exc + + try: + registry.add(package_id, entry) + except spec.registry_errors as exc: + # Fresh install: roll back the just-published directory so the system + # is not left with an unregistered package on disk. + try: + shutil.rmtree(destination) + except OSError as cleanup_exc: + raise spec.error( + f"Failed to update the {spec.lower} registry for '{package_id}': " + f"{exc}. The unregistered package remains at '{destination}' because " + f"rollback failed: {cleanup_exc}. Remove it manually before " + "reinstalling." + ) from exc + raise spec.error(str(exc)) from exc + + +def install_package( + request: InstallRequest, + spec: PackageSpec, + rules: PackageRules, + ops: StagingOps | None = None, +) -> dict[str, Any]: + """Validate, stage, and commit a package; return the persisted registry entry. + + The order is the contract: reject unsafe or conflicting targets, validate + the source before creating any project directory, copy it into a private + work directory, re-validate the complete staged copy (the source may have + changed while it was copied), then take the project lock and re-run every + conflict check against freshly read state immediately before publishing. + """ + ops = ops or StagingOps.standard(spec) + root, package_id, force = request.project_root, request.package_id, request.force + + package_dir = Path(request.package_dir) + if package_dir.is_symlink(): + raise spec.error( + f"Refusing to install from a symlinked package directory: '{package_dir}'" + ) + if not package_dir.is_dir(): + raise spec.error(f"{spec.noun} package directory not found: '{package_dir}'") + + location = rules.locate(root, package_id) + reject_source_overlap(package_dir, location.destination, spec) + registry = rules.open_registry(root) + rules.check_destination(registry, package_id, location, force=force) + + # Validate source and all caller-controlled metadata before creating any + # project directories. The staged metadata is used for the final entry. + manifest = rules.validate(package_dir, package_id) + rules.build_entry(manifest) + if rules.check_contents is not None: + rules.check_contents(registry, package_id, manifest, force=force) + + try: + location.base_dir.mkdir(parents=True, exist_ok=True) + work_dir = Path( + tempfile.mkdtemp(prefix=spec.work_prefix, dir=location.base_dir) + ) + except OSError as exc: + raise spec.error(f"Failed to create staging directory: {exc}") from exc + + staged_dir = work_dir / "staged" + committed = False + try: + ops.copy(package_dir, staged_dir) + staged_manifest = rules.validate(staged_dir, package_id) + entry = rules.build_entry(staged_manifest) + # Serialize destination and registry changes. Source downloads/copying + # stay outside the lock, but all state that can conflict is reloaded and + # checked again immediately before publication. + with ops.transaction(root): + locked = rules.locate(root, package_id) + if locked.base_dir != location.base_dir: + raise spec.error( + f"{spec.noun} directory changed while staging; reinstall from " + "the original source" + ) + registry = rules.open_registry(root) + rules.check_destination(registry, package_id, locked, force=force) + if rules.check_contents is not None: + rules.check_contents(registry, package_id, staged_manifest, force=force) + ops.publish( + locked.destination, staged_dir, registry, package_id, entry, force=force + ) + committed = True + finally: + primary_error = sys.exc_info()[1] + try: + shutil.rmtree(work_dir) + except OSError as cleanup_exc: + if work_dir.exists(): + message = ( + f"Could not remove {spec.lower} staging directory '{work_dir}': " + f"{cleanup_exc}" + ) + if primary_error is not None: + primary_error.add_note(message) + elif committed: + warnings.warn(message, UserWarning, stacklevel=2) + else: + raise spec.error(message) from cleanup_exc + + return entry + + +def remove_package( + request: RemoveRequest, spec: PackageSpec, hooks: RemoveHooks +) -> RemovalOutcome: + """Remove a package directory and its registry entry under the project lock. + + The registry and directory are resolved after the lock is acquired, so a + concurrent install cannot be lost or resurrected by a stale snapshot. The + registry entry is removed first and restored verbatim if deleting the + directory fails, so a later install is not blocked by an orphaned directory. + Failures raise ``spec.error``; advisory detail that should be shown + alongside it is attached as exception notes. + """ + root, package_id = request.project_root, request.package_id + transaction = hooks.transaction or functools.partial(install_transaction, spec=spec) + with contextlib.ExitStack() as stack: + try: + stack.enter_context(transaction(root)) + except spec.error as exc: + # Report the underlying acquisition error so the message is not + # prefixed twice by the helper's own lock-failure text. + raise spec.error( + f"Failed to lock {spec.lower} removal '{package_id}': " + f"{exc.__cause__ or exc}" + ) from exc + return remove_package_locked(request, spec, hooks) + + +def remove_package_locked( + request: RemoveRequest, spec: PackageSpec, hooks: RemoveHooks +) -> RemovalOutcome: + """The removal itself; the caller must already hold the project lock. + + Exposed for callers that run inside their own locked section (for example + a bundle refresh that snapshots and restores around the removal). + """ + root, package_id = request.project_root, request.package_id + registry = hooks.open_registry(root) + in_registry = registry.is_installed(package_id) + + base_dir = resolve_base_dir(root, spec) + destination = (base_dir / package_id).resolve() + # Defense-in-depth: even though the id was validated, ensure the resolved + # directory is a single child of the base directory and not the base itself. + try: + rel_parts = destination.relative_to(base_dir).parts + except ValueError: + raise spec.error(f"Invalid {spec.lower} id '{package_id}'") from None + if rel_parts != (package_id,): + raise spec.error(f"Invalid {spec.lower} id '{package_id}'") + + dir_exists = destination.exists() + if not in_registry and not dir_exists: + raise spec.error(f"{spec.kind_label} '{package_id}' is not installed") + + def _delete_failure(exc: OSError) -> Exception: + return spec.error( + f"Failed to remove {spec.lower} directory {destination}: {exc}" + ) + + if not in_registry: + # The registry was likely reset due to corruption. No registry write is + # needed; just delete the orphaned directory. + try: + shutil.rmtree(destination) + except OSError as exc: + failure = _delete_failure(exc) + failure.add_note(_orphan_note(package_id)) + raise failure from exc + return RemovalOutcome(destination, orphaned=True) + + metadata = registry.get(package_id) + try: + registry.remove(package_id) + except registry.ERROR as exc: + raise spec.error(str(exc)) from exc + if dir_exists: + try: + shutil.rmtree(destination) + except OSError as exc: + failure = _delete_failure(exc) + try: + if metadata is not None: + registry.restore(package_id, metadata) + except Exception as restore_exc: # noqa: BLE001 + failure.add_note( + f"Failed to restore registry entry for '{package_id}' after " + f"directory removal failure: {restore_exc}" + ) + raise failure from exc + return RemovalOutcome(destination, orphaned=False) + + +def _orphan_note(package_id: str) -> str: + return ( + f"'{package_id}' has no registry entry (registry may have been reset). " + "Removing the orphaned directory." + ) + + +def orphan_warning(package_id: str) -> str: + """Advisory shown when a removal deleted a directory that had no registry entry.""" + return _orphan_note(package_id) diff --git a/src/specify_cli/workflows/_package_registry.py b/src/specify_cli/workflows/_package_registry.py new file mode 100644 index 0000000000..1f2164b1c3 --- /dev/null +++ b/src/specify_cli/workflows/_package_registry.py @@ -0,0 +1,199 @@ +"""Persistent registry shared by installed workflow package kinds. + +Step packages and function packages are recorded the same way: a JSON file +beside the installed package directories, written atomically and never through +a symlinked path. A kind subclasses :class:`PackageRegistry` and names its +file, directory, record key and error type; persistence and record mutation +(including rollback of the in-memory state when a write fails) live here once. +""" + +from __future__ import annotations + +import copy +import json +import os +import stat +import tempfile +from datetime import UTC, datetime +from pathlib import Path +from typing import Any, ClassVar + + +class PackageRegistry: + """Registry of installed packages under ``.specify/workflows/``.""" + + REGISTRY_FILE: ClassVar[str] + RECORDS_KEY: ClassVar[str] # top-level key holding the id -> metadata mapping + DIRECTORY: ClassVar[str] # directory name under ``.specify/workflows`` + LABEL: ClassVar[str] # lowercase kind name used in messages + ERROR: ClassVar[type[Exception]] # raised when the registry cannot be written + SCHEMA_VERSION: ClassVar[str] = "1.0" + + def __init__(self, project_root: Path) -> None: + self.project_root = project_root + self.base_dir = project_root / ".specify" / "workflows" / self.DIRECTORY + self.registry_path = self.base_dir / self.REGISTRY_FILE + self.data = self._load() + + @property + def _records(self) -> dict[str, Any]: + return self.data[self.RECORDS_KEY] + + def _has_symlinked_parent(self) -> bool: + """Return True if any directory under .specify/workflows/ is a symlink.""" + current = self.project_root + for part in (".specify", "workflows", self.DIRECTORY): + current = current / part + if current.is_symlink(): + return True + return False + + def _load(self) -> dict[str, Any]: + """Load registry from disk or create default.""" + default_registry: dict[str, Any] = { + "schema_version": self.SCHEMA_VERSION, + self.RECORDS_KEY: {}, + } + # Defense-in-depth: refuse to read the registry if any parent directory + # is a symlink, which could redirect the read outside the project root. + if self._has_symlinked_parent(): + return default_registry + # Defense-in-depth: also refuse to read a symlinked registry file, + # which could redirect the read outside the project root. + if self.registry_path.is_symlink(): + return default_registry + if self.registry_path.exists(): + try: + with open(self.registry_path, encoding="utf-8") as f: + data = json.load(f) + # Validate shape: must be a dict with a dict records field + if not isinstance(data, dict): + return default_registry + if not isinstance(data.get(self.RECORDS_KEY), dict): + data[self.RECORDS_KEY] = {} + return data + except (json.JSONDecodeError, ValueError, OSError, UnicodeError): + return default_registry + return default_registry + + def save(self) -> None: + """Persist registry atomically without truncating an existing file.""" + if self._has_symlinked_parent() or self.registry_path.is_symlink(): + raise self.ERROR( + f"Refusing to write {self.LABEL} registry through a symlinked path." + ) + fd = -1 + tmp: str | None = None + try: + self.base_dir.mkdir(parents=True, exist_ok=True) + fd, tmp = tempfile.mkstemp( + dir=str(self.registry_path.parent), + prefix=f".{self.registry_path.name}.", + suffix=".tmp", + ) + # Keep the exclusive descriptor open while writing and checking the + # path so a replaced temporary file can never be committed. + with os.fdopen(os.dup(fd), "w", encoding="utf-8") as f: + json.dump(self.data, f, indent=2) + f.flush() + os.fsync(f.fileno()) + try: + if self.registry_path.exists(): + existing = self.registry_path.stat(follow_symlinks=False) + if stat.S_ISREG(existing.st_mode) and hasattr(os, "fchmod"): + os.fchmod(fd, stat.S_IMODE(existing.st_mode)) + if stat.S_ISREG(existing.st_mode) and hasattr(os, "fchown"): + try: + os.fchown(fd, existing.st_uid, existing.st_gid) + except PermissionError: + pass + except OSError: + # Persisting valid data is more important than preserving mode + # or ownership metadata when that best-effort operation fails. + pass + staged = os.stat(tmp, follow_symlinks=False) + opened = os.fstat(fd) + if ( + not stat.S_ISREG(staged.st_mode) + or staged.st_dev != opened.st_dev + or staged.st_ino != opened.st_ino + ): + raise OSError(f"Staged {self.LABEL} registry changed before commit") + os.close(fd) + fd = -1 + os.replace(tmp, self.registry_path) + tmp = None + except (OSError, TypeError, ValueError) as exc: + raise self.ERROR( + f"Failed to write {self.LABEL} registry at {self.registry_path}: {exc}" + ) from exc + finally: + if fd >= 0: + try: + os.close(fd) + except OSError: + pass + if tmp is not None: + try: + os.unlink(tmp) + except OSError: + pass + + def add(self, package_id: str, metadata: dict[str, Any]) -> None: + """Add or update an installed package entry.""" + records = self._records + raw_existing = records.get(package_id) + had_entry = package_id in records + # Corrupted-but-parseable registries may hold non-dict entries; treat + # them as absent rather than crashing on existing.get() (mirrors + # WorkflowRegistry.add). + existing = raw_existing if isinstance(raw_existing, dict) else {} + metadata_to_store = copy.deepcopy(metadata) + metadata_to_store["installed_at"] = existing.get( + "installed_at", datetime.now(UTC).isoformat() + ) + metadata_to_store["updated_at"] = datetime.now(UTC).isoformat() + records[package_id] = metadata_to_store + try: + self.save() + except (self.ERROR, TypeError, ValueError): + if had_entry: + records[package_id] = raw_existing + else: + del records[package_id] + raise + + def remove(self, package_id: str) -> bool: + """Remove an installed package entry. Returns True if found.""" + records = self._records + if package_id in records: + removed_entry = records[package_id] + del records[package_id] + try: + self.save() + except (self.ERROR, TypeError, ValueError): + records[package_id] = removed_entry + raise + return True + return False + + def restore(self, package_id: str, metadata: dict[str, Any]) -> None: + """Put back a previously removed entry verbatim and persist it. + + Used to undo a removal whose filesystem step failed; unlike ``add`` it + keeps the original timestamps. + """ + self._records[package_id] = metadata + self.save() + + def get(self, package_id: str) -> dict[str, Any] | None: + """Get metadata for an installed package.""" + return self._records.get(package_id) + + def list(self) -> dict[str, dict[str, Any]]: + """Return all installed packages.""" + return dict(self._records) + + def is_installed(self, package_id: str) -> bool: + """Check if a package is installed.""" + return package_id in self._records diff --git a/src/specify_cli/workflows/step/catalog/_domain.py b/src/specify_cli/workflows/step/catalog/_domain.py index 50e79daa47..b676a4077d 100644 --- a/src/specify_cli/workflows/step/catalog/_domain.py +++ b/src/specify_cli/workflows/step/catalog/_domain.py @@ -6,11 +6,8 @@ import json import logging import os -import stat -import tempfile import time from dataclasses import dataclass -from datetime import UTC from pathlib import Path from typing import Any @@ -20,6 +17,7 @@ read_response_limited, ) from ..._catalog_cache import read_cache_json, write_cache_json +from ..._package_registry import PackageRegistry logger = logging.getLogger(__name__) @@ -78,170 +76,24 @@ class StepCatalogEntry: # --------------------------------------------------------------------------- -class StepRegistry: +class StepRegistry(PackageRegistry): """Manages the registry of installed custom step types. Tracks installed step types and their metadata in - ``.specify/workflows/steps/step-registry.json``. + ``.specify/workflows/steps/step-registry.json``. Persistence and record + mutation are shared with function packages in + :class:`~specify_cli.workflows._package_registry.PackageRegistry`. """ REGISTRY_FILE = "step-registry.json" - SCHEMA_VERSION = "1.0" - - def __init__(self, project_root: Path) -> None: - self.project_root = project_root - self.steps_dir = project_root / ".specify" / "workflows" / "steps" - self.registry_path = self.steps_dir / self.REGISTRY_FILE - self.data = self._load() - - def _has_symlinked_parent(self) -> bool: - """Return True if any directory under .specify/workflows/steps is a symlink.""" - current = self.project_root - for part in (".specify", "workflows", "steps"): - current = current / part - if current.is_symlink(): - return True - return False - - def _load(self) -> dict[str, Any]: - """Load registry from disk or create default.""" - default_registry: dict[str, Any] = {"schema_version": self.SCHEMA_VERSION, "steps": {}} - # Defense-in-depth: refuse to read the registry if any parent directory - # under .specify/workflows/steps is a symlink, which could redirect the - # read outside the project root. - if self._has_symlinked_parent(): - return default_registry - # Defense-in-depth: also refuse to read a symlinked registry file, - # which could redirect the read outside the project root. - if self.registry_path.is_symlink(): - return default_registry - if self.registry_path.exists(): - try: - with open(self.registry_path, encoding="utf-8") as f: - data = json.load(f) - # Validate shape: must be a dict with a dict "steps" field - if not isinstance(data, dict): - return default_registry - if not isinstance(data.get("steps"), dict): - data["steps"] = {} - return data - except (json.JSONDecodeError, ValueError, OSError, UnicodeError): - return default_registry - return default_registry - - def save(self) -> None: - """Persist registry atomically without truncating an existing file.""" - if self._has_symlinked_parent() or self.registry_path.is_symlink(): - raise StepValidationError( - "Refusing to write step registry through a symlinked path." - ) - fd = -1 - tmp: str | None = None - try: - self.steps_dir.mkdir(parents=True, exist_ok=True) - fd, tmp = tempfile.mkstemp( - dir=str(self.registry_path.parent), - prefix=f".{self.registry_path.name}.", - suffix=".tmp", - ) - # Keep the exclusive descriptor open while writing and checking the - # path so a replaced temporary file can never be committed. - with os.fdopen(os.dup(fd), "w", encoding="utf-8") as f: - json.dump(self.data, f, indent=2) - f.flush() - os.fsync(f.fileno()) - try: - if self.registry_path.exists(): - existing = self.registry_path.stat(follow_symlinks=False) - if stat.S_ISREG(existing.st_mode) and hasattr(os, "fchmod"): - os.fchmod(fd, stat.S_IMODE(existing.st_mode)) - if stat.S_ISREG(existing.st_mode) and hasattr(os, "fchown"): - try: - os.fchown(fd, existing.st_uid, existing.st_gid) - except PermissionError: - pass - except OSError: - # Persisting valid data is more important than preserving mode - # or ownership metadata when that best-effort operation fails. - pass - staged = os.stat(tmp, follow_symlinks=False) - opened = os.fstat(fd) - if ( - not stat.S_ISREG(staged.st_mode) - or staged.st_dev != opened.st_dev - or staged.st_ino != opened.st_ino - ): - raise OSError("Staged step registry changed before commit") - os.close(fd) - fd = -1 - os.replace(tmp, self.registry_path) - tmp = None - except (OSError, TypeError, ValueError) as exc: - raise StepValidationError( - f"Failed to write step registry at {self.registry_path}: {exc}" - ) from exc - finally: - if fd >= 0: - try: - os.close(fd) - except OSError: - pass - if tmp is not None: - try: - os.unlink(tmp) - except OSError: - pass - - def add(self, step_id: str, metadata: dict[str, Any]) -> None: - """Add or update an installed step entry.""" - import copy - from datetime import datetime - - raw_existing = self.data["steps"].get(step_id) - had_entry = step_id in self.data["steps"] - # Corrupted-but-parseable registries may hold non-dict entries; treat - # them as absent rather than crashing on existing.get() (mirrors - # WorkflowRegistry.add). - existing = raw_existing if isinstance(raw_existing, dict) else {} - metadata_to_store = copy.deepcopy(metadata) - metadata_to_store["installed_at"] = existing.get( - "installed_at", datetime.now(UTC).isoformat() - ) - metadata_to_store["updated_at"] = datetime.now(UTC).isoformat() - self.data["steps"][step_id] = metadata_to_store - try: - self.save() - except (StepValidationError, TypeError, ValueError): - if had_entry: - self.data["steps"][step_id] = raw_existing - else: - del self.data["steps"][step_id] - raise - - def remove(self, step_id: str) -> bool: - """Remove an installed step entry. Returns True if found.""" - if step_id in self.data["steps"]: - removed_entry = self.data["steps"][step_id] - del self.data["steps"][step_id] - try: - self.save() - except (StepValidationError, TypeError, ValueError): - self.data["steps"][step_id] = removed_entry - raise - return True - return False - - def get(self, step_id: str) -> dict[str, Any] | None: - """Get metadata for an installed step.""" - return self.data["steps"].get(step_id) - - def list(self) -> dict[str, dict[str, Any]]: - """Return all installed steps.""" - return dict(self.data["steps"]) - - def is_installed(self, step_id: str) -> bool: - """Check if a step is installed.""" - return step_id in self.data["steps"] + RECORDS_KEY = "steps" + DIRECTORY = "steps" + LABEL = "step" + ERROR = StepValidationError + + @property + def steps_dir(self) -> Path: + return self.base_dir # --------------------------------------------------------------------------- diff --git a/src/specify_cli/workflows/step/command_add.py b/src/specify_cli/workflows/step/command_add.py index 517fd6126c..91762f0665 100644 --- a/src/specify_cli/workflows/step/command_add.py +++ b/src/specify_cli/workflows/step/command_add.py @@ -19,24 +19,6 @@ _MAX_STEP_CATALOG_RESPONSE_BYTES = 50 * 1024 * 1024 -def _cleanup_download_tmp_path(tmp_path: cli.Path | None) -> None: - """Best-effort unlink of a partially-downloaded step archive temp file. - - A cleanup ``OSError`` must never replace/mask whatever error or interrupt is - already propagating -- warn about it and keep going. - """ - if tmp_path is None: - return - try: - tmp_path.unlink(missing_ok=True) - except OSError as cleanup_exc: - cli.console.print( - "[yellow]Warning:[/yellow] Could not remove temporary " - f"step download file: {cli._escape_markup(str(cleanup_exc))} " - f"(path: {cli._escape_markup(str(tmp_path))})" - ) - - def _print_installed(step_id: str, entry: dict) -> None: step_name = entry.get("name") or step_id cli.console.print( @@ -76,185 +58,22 @@ def _install_from_url( project_root: cli.Path, step_id: str, from_url: str, *, force: bool ) -> None: """Install a step package archive from a direct URL.""" - import tempfile - from urllib.parse import urlparse - - from rich.panel import Panel - - from specify_cli.authentication.github_http import ( - resolve_github_release_asset_api_url as _resolve_gh_asset, - ) - from specify_cli.authentication.http import ( - github_provider_hosts as _github_provider_hosts, - ) - from specify_cli.authentication.http import open_url as _open_url - + from .. import _archive_install as archive from . import installer - try: - parsed = urlparse(from_url) - hostname = parsed.hostname - _ = parsed.port - except ValueError: - raise installer.StepInstallError( - f"Invalid URL: {from_url}" - ) from None - if not hostname: - raise installer.StepInstallError( - f"Invalid URL: {from_url}" - ) - if not cli.is_https_or_localhost_http(from_url): - raise installer.StepInstallError( - "URL must use HTTPS for security. HTTP is only allowed for " - "loopback URLs." - ) - - # Reject before the trust prompt and before any network request. - installer.check_installable(project_root, step_id, force=force) - - # Prompt BEFORE any request (and before any spinner) so the user can see - # and answer it; a declined prompt issues no request and exits 0. - cli.console.print() - cli.console.print( - Panel( - "[bold]You are installing a workflow step type directly from an " - "external URL.\nA step package contains executable Python.[/bold]\n\n" - f"URL: {cli._escape_markup(from_url)}\n\n" - "Only install step packages from sources you trust.", - title="[bold yellow]⚠ Untrusted Source[/bold yellow]", - border_style="yellow", - padding=(1, 2), - ) + entry = archive.install_from_archive_url( + archive.ArchiveRequest(project_root, step_id, from_url, force), + installer._step_spec(), + archive.ArchiveHooks( + preflight=lambda root, sid, force: installer.check_installable( + root, sid, force=force + ), + resolve_root=lambda extracted: installer.resolve_package_root(extracted), + install=lambda root, sid, package, force: installer.install_step_package( + root, sid, package, source="url", force=force + ), + ), ) - cli.console.print() - if not cli.typer.confirm("Continue with installation?", default=False): - cli.console.print("Cancelled") - raise cli.typer.Exit(0) - - download_url = from_url - extra_headers = None - tmp_path: cli.Path | None = None - extract_tmp: tempfile.TemporaryDirectory[str] | None = None - committed = False - try: - resolved_url = _resolve_gh_asset( - from_url, - _open_url, - timeout=30, - github_hosts=_github_provider_hosts(), - redirect_validator=cli._reject_insecure_download_redirect, - ) - if resolved_url: - download_url = resolved_url - extra_headers = {"Accept": "application/octet-stream"} - - with _open_url( - download_url, - timeout=30, - extra_headers=extra_headers, - redirect_validator=cli._reject_insecure_download_redirect, - ) as resp: - final_url = resp.geturl() - if not cli.is_https_or_localhost_http(final_url): - raise installer.StepInstallError( - f"URL redirected to non-HTTPS: {final_url}" - ) - content_type = ( - resp.getheader("Content-Type") - if hasattr(resp, "getheader") - else None - ) - declarations = [ - ("requested URL", from_url, cli.archive_format_from_name(from_url)), - ("final URL", final_url, cli.archive_format_from_name(final_url)), - ( - "Content-Type", - content_type or "", - cli.archive_format_from_content_type(content_type), - ), - ] - recognized = [item for item in declarations if item[2] is not None] - archive_format = recognized[0][2] if recognized else None - if any(item[2] != archive_format for item in recognized): - details = ", ".join( - f"{label} declares {declared}" - for label, _value, declared in recognized - ) - raise installer.StepInstallError( - f"Archive format mismatch: {details}" - ) - downloaded = cli.read_response_limited( - resp, - error_type=ValueError, - label="step archive download", - ) - - with tempfile.NamedTemporaryFile( - suffix=cli.archive_suffix(archive_format) if archive_format else ".archive", - delete=False, - ) as tmp: - tmp_path = cli.Path(tmp.name) - tmp.write(downloaded) - - try: - extract_tmp = tempfile.TemporaryDirectory( - prefix="speckit-step-archive-" - ) - except OSError as exc: - raise installer.StepInstallError( - f"Failed to create temporary step archive directory: {exc}" - ) from exc - extracted_root = cli.Path(extract_tmp.name) - try: - # safe_extract_archive re-detects and confirms the archive bytes. - cli.safe_extract_archive( - tmp_path, - extracted_root, - source_name=recognized[0][1] if recognized else None, - content_type=content_type, - ) - package_root = installer.resolve_package_root(extracted_root) - entry = installer.install_step_package( - project_root, - step_id, - package_root, - source="url", - force=force, - ) - committed = True - finally: - primary_error = sys.exc_info()[1] - try: - extract_tmp.cleanup() - except OSError as cleanup_exc: - if extract_tmp.name and cli.Path(extract_tmp.name).exists(): - detail = ( - f"{cli._escape_markup(str(cleanup_exc))} " - f"(path: {cli._escape_markup(extract_tmp.name)})" - ) - cli.console.print( - "[yellow]Warning:[/yellow] Could not remove temporary " - f"step archive directory: {detail}" - ) - if primary_error is None and not committed: - raise installer.StepInstallError( - "Failed to remove temporary step archive directory: " - f"{cleanup_exc}" - ) from cleanup_exc - # Do not raise from cleanup: the primary installation error - # (if any) is already propagating, and after commit the install - # has succeeded. The warning above reports the residual path. - except cli.typer.Exit: - raise - except installer.StepInstallError: - raise - except Exception as exc: - raise installer.StepInstallError( - f"Failed to install step from URL: {exc}" - ) from exc - finally: - _cleanup_download_tmp_path(tmp_path) - _print_installed(step_id, entry) @@ -598,10 +417,5 @@ def workflow_step_add( else: _install_from_catalog(project_root, step_id, force=force, version=version) except installer.StepInstallError as exc: - notes = getattr(exc, "__notes__", ()) - for note in notes: - cli.console.print( - f"[yellow]Warning:[/yellow] {cli._escape_markup(note)}" - ) - cli.console.print(f"[red]Error:[/red] {cli._escape_markup(str(exc))}") + cli._print_package_error(exc) raise cli.typer.Exit(1) from exc diff --git a/src/specify_cli/workflows/step/command_remove.py b/src/specify_cli/workflows/step/command_remove.py index 52a9a08a6b..6c44c38bf7 100644 --- a/src/specify_cli/workflows/step/command_remove.py +++ b/src/specify_cli/workflows/step/command_remove.py @@ -8,93 +8,45 @@ from . import _helpers as step_helpers +def _step_remove_hooks(installer): + from .. import _package_install as package_install + + return package_install.RemoveHooks( + open_registry=lambda root: installer._open_registry(root), + transaction=lambda root: installer._step_install_transaction(root), + ) + + +def _print_orphan_warning(step_id: str) -> None: + from .. import _package_install as package_install + + cli.console.print( + "[yellow]Warning:[/yellow] " + f"{cli._escape_markup(package_install.orphan_warning(step_id))}" + ) + + def _remove_step_locked(project_root: cli.Path, step_id: str) -> None: """Remove a step's registry entry and directory while the lock is held. The registry and step directory are resolved here, after the lock is acquired, so a concurrent install cannot be lost or resurrected by a stale - registry snapshot. + registry snapshot. Domain failures are rendered and exit with code 1. """ - import shutil + from .. import _package_install as package_install + from . import installer - from .catalog import StepRegistry, StepValidationError - - safe_step_id = cli._escape_markup(step_id) - registry = StepRegistry(project_root) - in_registry = registry.is_installed(step_id) - - steps_base_dir = step_helpers._resolve_steps_base_dir_or_exit(project_root) - step_dir = (steps_base_dir / step_id).resolve() - # Defense-in-depth: even though step_helpers._validate_step_id_or_exit rejects path - # separators, ensure that the resolved directory is a single child of - # steps_base_dir and is not steps_base_dir itself. try: - rel_parts = step_dir.relative_to(steps_base_dir).parts - except ValueError: - cli.console.print(f"[red]Error:[/red] Invalid step id '{safe_step_id}'") - raise cli.typer.Exit(1) - if rel_parts != (step_id,): - cli.console.print(f"[red]Error:[/red] Invalid step id '{safe_step_id}'") - raise cli.typer.Exit(1) - - dir_exists = step_dir.exists() - - if not in_registry and not dir_exists: - cli.console.print(f"[red]Error:[/red] Step type '{safe_step_id}' is not installed") - raise cli.typer.Exit(1) - - if not in_registry and dir_exists: - # The registry was likely reset due to corruption. Warn the user that the - # directory is being removed even though there is no registry entry, so - # the orphaned package can be cleaned up and a fresh install attempted. - cli.console.print( - f"[yellow]Warning:[/yellow] '{safe_step_id}' has no registry entry " - "(registry may have been reset). Removing the orphaned directory." + outcome = package_install.remove_package_locked( + package_install.RemoveRequest(project_root, step_id), + installer._step_spec(), + _step_remove_hooks(installer), ) - - if dir_exists and not in_registry: - # No registry write needed; just delete the orphaned directory. - try: - shutil.rmtree(step_dir) - except OSError as exc: - cli.console.print( - "[red]Error:[/red] Failed to remove step directory " - f"{cli._escape_markup(str(step_dir))}: {cli._escape_markup(str(exc))}" - ) - raise cli.typer.Exit(1) - elif in_registry: - # Remove the registry entry, then the directory. If the directory - # delete fails, restore the registry entry so state stays consistent - # and a future `step add` isn't blocked by an orphaned directory - # with no registry entry. - registry_metadata = registry.get(step_id) - try: - registry.remove(step_id) - except StepValidationError as exc: - cli.console.print(f"[red]Error:[/red] {cli._escape_markup(str(exc))}") - raise cli.typer.Exit(1) - if dir_exists: - try: - shutil.rmtree(step_dir) - except OSError as exc: - # Restore the original registry entry verbatim (bypass add() - # which would overwrite timestamps). - try: - if registry_metadata is not None: - registry.data["steps"][step_id] = registry_metadata - registry.save() - except Exception as restore_exc: # noqa: BLE001 - cli.console.print( - f"[yellow]Warning:[/yellow] Failed to restore registry entry " - f"for '{safe_step_id}' after directory removal failure: " - f"{cli._escape_markup(str(restore_exc))}" - ) - cli.console.print( - "[red]Error:[/red] Failed to remove step directory " - f"{cli._escape_markup(str(step_dir))}: " - f"{cli._escape_markup(str(exc))}" - ) - raise cli.typer.Exit(1) + except installer.StepInstallError as exc: + cli._print_package_error(exc) + raise cli.typer.Exit(1) from exc + if outcome.orphaned: + _print_orphan_warning(step_id) @step_app.command("remove") @@ -102,7 +54,8 @@ def workflow_step_remove( step_id: str = cli.typer.Argument(..., help="Step type ID to uninstall"), ): """Uninstall a custom step type.""" - from .installer import StepInstallError, _step_install_transaction + from .. import _package_install as package_install + from . import installer project_root = cli._require_specify_project() @@ -111,19 +64,17 @@ def workflow_step_remove( # Removal mutates the same directory and registry as `step add`, so it # shares the install lock to prevent lost or resurrected registry entries. try: - with _step_install_transaction(project_root): - _remove_step_locked(project_root, step_id) - except StepInstallError as exc: - # Report the underlying acquisition error so the message is not - # prefixed twice by the helper's own lock-failure text. - cause = exc.__cause__ or exc - cli.console.print( - "[red]Error:[/red] Failed to lock step removal " - f"'{cli._escape_markup(step_id)}': " - f"{cli._escape_markup(str(cause))}" + outcome = package_install.remove_package( + package_install.RemoveRequest(project_root, step_id), + installer._step_spec(), + _step_remove_hooks(installer), ) - raise cli.typer.Exit(1) + except installer.StepInstallError as exc: + cli._print_package_error(exc) + raise cli.typer.Exit(1) from exc + if outcome.orphaned: + _print_orphan_warning(step_id) cli.console.print( f"[green]✓[/green] Step type '{cli._escape_markup(step_id)}' uninstalled" ) diff --git a/src/specify_cli/workflows/step/installer.py b/src/specify_cli/workflows/step/installer.py index de55d1f39d..f12f275a15 100644 --- a/src/specify_cli/workflows/step/installer.py +++ b/src/specify_cli/workflows/step/installer.py @@ -1,35 +1,37 @@ """Domain install/validation for custom workflow step packages. -This module owns the source-independent behavior shared by every +This module owns the step-specific behavior shared by every ``specify workflow step add`` source mode (catalog, ``--dev`` local directory, -and ``--from`` archive URL): step-id and base-directory validation, package -shape/symlink/limit validation, staging, atomic commit, and registry -provenance. It is deliberately CLI-independent -- it never prints and never -raises ``typer.Exit``. Callers receive :class:`StepInstallError` and decide how -to surface it. +and ``--from`` archive URL): step-id validation, ``step.yml`` parsing, +registry entries and built-in collisions. The kind-independent mechanics +(directory and symlink safety, package-tree walking and copying, the staged +install flow, locking and replacement) live in +:mod:`specify_cli.workflows._package_install`; the functions below keep the +established step names as thin bindings to it so existing patch points and +messages are unchanged. It is deliberately CLI-independent -- it never prints +and never raises ``typer.Exit``. Callers receive :class:`StepInstallError` and +decide how to surface it. """ from __future__ import annotations import contextlib import os -import shutil -import stat -import sys -import tempfile -import warnings +import shutil # noqa: F401 -- ``installer.shutil`` is an established patch point from collections.abc import Mapping from pathlib import Path -from typing import Any +from typing import TYPE_CHECKING, Any import yaml +if TYPE_CHECKING: + from .._package_install import PackageLocation, PackageSpec + # Custom step packages contain executable Python, metadata, and optional helper # files. These ceilings apply uniformly to catalog, local, and archive sources. _MAX_STEP_PACKAGE_FILES = 512 _MAX_STEP_PACKAGE_BYTES = 50 * 1024 * 1024 # 50 MiB _MAX_STEP_PACKAGE_DEPTH = 32 -_COPY_CHUNK_BYTES = 64 * 1024 # Files/dirs never inspected, copied into, or counted as part of an installed # step package; excluded directories are pruned without being entered. Mirrors @@ -74,29 +76,47 @@ _WINDOWS_INVALID_CHARS: frozenset[str] = frozenset('<>:"|?*') _SOURCES: frozenset[str] = frozenset({"catalog", "local", "url"}) - class StepInstallError(Exception): """User-facing step package install/validation failure.""" + +def _pi(): + """The shared install module, imported on use. + + ``.github/scripts/validate_community_workflow_step.py`` loads this file on + its own (outside the package) to reuse the id validator and the limits, so + nothing relative may be imported at module level. + """ + from .. import _package_install + + return _package_install + + +def _step_spec() -> PackageSpec: + """Build the step spec from the module limits at call time. + + Read at call time so a changed ``_MAX_STEP_PACKAGE_*`` is honored. + """ + from .catalog import StepValidationError + + shared = _pi() + return shared.PackageSpec( + noun="Step", + kind_label="Step type", + error=StepInstallError, + limits=shared.PackageLimits( + _MAX_STEP_PACKAGE_FILES, _MAX_STEP_PACKAGE_BYTES, _MAX_STEP_PACKAGE_DEPTH + ), + registry_errors=(StepValidationError, OSError, TypeError, ValueError), + excludes=EXCLUDE_NAMES, + ) + + @contextlib.contextmanager def _step_install_transaction(project_root: Path): """Serialize step directory and registry mutations (install and remove).""" - from ...shared_infra import _exclusive_project_lock - - # Only acquisition failures are reported as lock errors; exceptions raised - # by the caller's critical section propagate unchanged. The message is - # operation-neutral because both install and remove share this lock; callers - # that add their own context should report ``exc.__cause__`` instead. - with contextlib.ExitStack() as stack: - try: - stack.enter_context( - _exclusive_project_lock( - Path(project_root), ".step-install.lock", context="step" - ) - ) - except OSError as exc: - raise StepInstallError(f"Failed to acquire the step lock: {exc}") from exc + with _pi().install_transaction(project_root, _step_spec()): yield @@ -135,57 +155,20 @@ def validate_step_id(step_id: str) -> None: ) + def resolve_steps_base_dir(project_root: Path) -> Path: """Resolve ``.specify/workflows/steps`` refusing symlinked parent dirs.""" - project_root = Path(project_root) - project_root_resolved = project_root.resolve() - steps_base_dir_unresolved = project_root / ".specify" / "workflows" / "steps" - - current = project_root - for part in (".specify", "workflows", "steps"): - current = current / part - if current.is_symlink(): - raise StepInstallError( - f"Refusing to use symlinked step directory '{current}'" - ) - if current.exists() and not current.is_dir(): - raise StepInstallError( - f"Step directory path is not a directory: '{current}'" - ) - - steps_base_dir = steps_base_dir_unresolved.resolve() - try: - steps_base_dir.relative_to(project_root_resolved) - except ValueError: - raise StepInstallError( - f"Step directory escapes project root: '{steps_base_dir}'" - ) from None - - return steps_base_dir + return _pi().resolve_base_dir(project_root, _step_spec()) def _resolve_step_dir(steps_base_dir: Path, step_id: str) -> Path: """Return the canonical destination directory for ``step_id``.""" - step_dir = steps_base_dir / step_id - try: - rel_parts = step_dir.relative_to(steps_base_dir).parts - except ValueError: - raise StepInstallError(f"Invalid step id '{step_id}'") from None - if rel_parts != (step_id,): - raise StepInstallError(f"Invalid step id '{step_id}'") - return step_dir + return _pi().resolve_destination(steps_base_dir, step_id, _step_spec()) def _reject_unsafe_destination(step_dir: Path) -> None: """Refuse a symlink (including dangling) or non-directory destination.""" - if step_dir.is_symlink(): - raise StepInstallError( - f"Refusing to install step through a symlinked path: '{step_dir}'" - ) - if step_dir.exists() and not step_dir.is_dir(): - raise StepInstallError( - f"Step install path exists but is not a directory: '{step_dir}'" - ) + _pi().reject_unsafe_destination(step_dir, _step_spec()) # --------------------------------------------------------------------------- @@ -194,86 +177,16 @@ def _reject_unsafe_destination(step_dir: Path) -> None: def _entry_limit_error() -> StepInstallError: - return StepInstallError( - "Step package contains too many entries, exceeding the " - f"{_MAX_STEP_PACKAGE_FILES}-entry limit (files and directories combined)" - ) + return _pi().entry_limit_error(_step_spec()) def _scan_retained_entries(directory: Path, budget: int) -> list[os.DirEntry]: - """Return *directory*'s retained children, sorted by name. - - Streams the listing and drops ``EXCLUDE_NAMES`` without inspecting them. - Raises the entry-limit error as soon as more than *budget* retained - entries are seen, so an oversized directory is never fully materialized - or sorted. ``OSError`` propagates to the caller. - """ - entries: list[os.DirEntry] = [] - with os.scandir(directory) as iterator: - for entry in iterator: - if entry.name in EXCLUDE_NAMES: - continue - if len(entries) >= budget: - raise _entry_limit_error() - entries.append(entry) - entries.sort(key=lambda entry: entry.name) - return entries + return _pi().scan_retained_entries(directory, budget, _step_spec()) def _walk_package_tree(package_dir: Path): - """Yield ``(path, is_dir)`` for every retained descendant of *package_dir*. - - Excluded names are pruned without being entered or inspected, matching - ``bundles/packager.py``; their contents are never staged, so they are not - part of the package. Every retained entry discovered counts against - ``_MAX_STEP_PACKAGE_FILES`` before its directory listing is sorted, which - bounds traversal work by the entry ceiling. Never follows a symlink. - Raises :class:`StepInstallError` on any symlink, object that is neither a - regular file nor a directory, nesting deeper than - ``_MAX_STEP_PACKAGE_DEPTH``, or too many entries. Iterative so deep trees - cannot exhaust the Python recursion limit. - """ - discovered = 0 - - def _children(current: Path, depth: int): - nonlocal discovered - try: - entries = _scan_retained_entries( - current, _MAX_STEP_PACKAGE_FILES - discovered - ) - except OSError as exc: - raise StepInstallError( - f"Failed to read step package directory '{current}': {exc}" - ) from exc - discovered += len(entries) - return [(entry, depth) for entry in reversed(entries)] - - stack = _children(package_dir, 1) - while stack: - entry, depth = stack.pop() - try: - mode = entry.stat(follow_symlinks=False).st_mode - except OSError as exc: - raise StepInstallError( - f"Failed to inspect step package entry '{entry.path}': {exc}" - ) from exc - path = Path(entry.path) - if stat.S_ISLNK(mode): - raise StepInstallError(f"Step package contains symlink: {path}") - if stat.S_ISDIR(mode): - if depth > _MAX_STEP_PACKAGE_DEPTH: - raise StepInstallError( - f"Step package exceeds the {_MAX_STEP_PACKAGE_DEPTH}-level " - "directory depth limit" - ) - yield path, True - stack.extend(_children(path, depth + 1)) - elif stat.S_ISREG(mode): - yield path, False - else: - raise StepInstallError( - f"Step package contains unsupported file: {path}" - ) + """Yield ``(path, is_dir)`` for every retained descendant of *package_dir*.""" + return _pi().walk_package_tree(package_dir, _step_spec()) def _parse_step_metadata(step_yml_text: str, step_id: str) -> dict[str, Any]: @@ -324,41 +237,7 @@ def validate_step_package(package_dir: Path, step_id: str) -> dict[str, Any]: violation. Never imports or executes ``__init__.py``. """ package_dir = Path(package_dir) - - if package_dir.is_symlink(): - raise StepInstallError( - f"Refusing to install from a symlinked package directory: '{package_dir}'" - ) - if not package_dir.is_dir(): - raise StepInstallError(f"Step package directory not found: '{package_dir}'") - - for required in ("step.yml", "__init__.py"): - required_path = package_dir / required - if required_path.is_symlink(): - raise StepInstallError( - f"Step package '{required}' must be a regular file, not a symlink" - ) - if not required_path.is_file(): - raise StepInstallError( - f"Step package is missing required file '{required}' at its root" - ) - - # The walk enforces the entry ceiling itself; only bytes are summed here. - retained_bytes = 0 - for path, is_dir in _walk_package_tree(package_dir): - if is_dir: - continue - try: - retained_bytes += path.lstat().st_size - except OSError as exc: - raise StepInstallError( - f"Failed to inspect step package file '{path}': {exc}" - ) from exc - if retained_bytes > _MAX_STEP_PACKAGE_BYTES: - raise StepInstallError( - f"Step package exceeds the {_MAX_STEP_PACKAGE_BYTES}-byte total " - "size limit" - ) + _pi().check_package_shape(package_dir, _step_spec(), ("step.yml", "__init__.py")) try: step_yml_text = (package_dir / "step.yml").read_text(encoding="utf-8") @@ -370,30 +249,7 @@ def validate_step_package(package_dir: Path, step_id: str) -> dict[str, Any]: def resolve_package_root(extracted_root: Path) -> Path: """Resolve a root-level or single-nested step package directory.""" - extracted_root = Path(extracted_root) - root_manifest = extracted_root / "step.yml" - if root_manifest.is_file() and not root_manifest.is_symlink(): - return extracted_root - try: - entries = list(extracted_root.iterdir()) - except OSError as exc: - raise StepInstallError( - f"Failed to inspect archive contents: {exc}" - ) from exc - if len(entries) == 1: - candidate = entries[0] - candidate_manifest = candidate / "step.yml" - if ( - candidate.is_dir() - and not candidate.is_symlink() - and candidate_manifest.is_file() - and not candidate_manifest.is_symlink() - ): - return candidate - raise StepInstallError( - "archive must contain step.yml at its root or in exactly one top-level " - "directory" - ) + return _pi().resolve_package_root(extracted_root, _step_spec(), "step.yml") # --------------------------------------------------------------------------- @@ -410,60 +266,33 @@ def _reject_builtin_collision(step_id: str) -> None: ) + def _check_duplicate( registry: Any, step_id: str, step_dir: Path, *, force: bool ) -> None: - folded_id = step_id.casefold() - try: - registered_ids = registry.list() - except (AttributeError, TypeError): - registered_ids = () - for registered_id in registered_ids: - if ( - isinstance(registered_id, str) - and registered_id != step_id - and registered_id.casefold() == folded_id - ): - raise StepInstallError( - f"Step ID '{step_id}' collides case-insensitively with registered " - f"step ID '{registered_id}'; use the exact registered ID with --force" - ) + _pi().check_duplicate(registry, step_id, step_dir, force=force, spec=_step_spec()) - try: - destination_exists = step_dir.exists() - for existing_path in step_dir.parent.iterdir(): - existing_name = existing_path.name - if existing_name == step_id: - continue - aliases_destination = ( - destination_exists and existing_path.samefile(step_dir) - ) - if existing_name.casefold() != folded_id and not aliases_destination: - continue - raise StepInstallError( - f"Step ID '{step_id}' collides case-insensitively or resolves to " - f"the same filesystem path as existing step directory " - f"'{existing_name}'" - ) - except FileNotFoundError: - pass - except OSError as exc: - raise StepInstallError( - f"Failed to inspect existing step directories: {exc}" - ) from exc - if force: - return - if registry.is_installed(step_id): - raise StepInstallError( - f"Step type '{step_id}' is already installed. Remove it first with: " - f"specify workflow step remove {step_id}" - ) - if step_dir.exists(): - raise StepInstallError( - f"Step directory already exists at '{step_dir}'. Remove it manually " - f"or use: specify workflow step remove {step_id}" - ) +def _locate(project_root: Path, step_id: str) -> PackageLocation: + """Validate the id and resolve its base directory and destination slot.""" + validate_step_id(step_id) + steps_base_dir = resolve_steps_base_dir(project_root) + step_dir = _resolve_step_dir(steps_base_dir, step_id) + _reject_unsafe_destination(step_dir) + return _pi().PackageLocation(steps_base_dir, step_dir) + + +def _open_registry(project_root: Path): + from .catalog import StepRegistry + + return StepRegistry(project_root) + + +def _check_destination( + registry: Any, step_id: str, location: PackageLocation, *, force: bool +) -> None: + _reject_builtin_collision(step_id) + _check_duplicate(registry, step_id, location.destination, force=force) def check_installable(project_root: Path, step_id: str, *, force: bool = False) -> Path: @@ -474,16 +303,9 @@ def check_installable(project_root: Path, step_id: str, *, force: bool = False) uses this to reject before a download; :func:`install_step_package` re-runs the same checks as defense-in-depth. """ - from .catalog import StepRegistry - - validate_step_id(step_id) - steps_base_dir = resolve_steps_base_dir(project_root) - step_dir = _resolve_step_dir(steps_base_dir, step_id) - _reject_unsafe_destination(step_dir) - _reject_builtin_collision(step_id) - registry = StepRegistry(project_root) - _check_duplicate(registry, step_id, step_dir, force=force) - return step_dir + location = _locate(project_root, step_id) + _check_destination(_open_registry(project_root), step_id, location, force=force) + return location.destination # --------------------------------------------------------------------------- @@ -551,111 +373,24 @@ def _string_value(metadata: Mapping[str, Any], field: str) -> str | None: return entry -def _copy_package_tree(source_dir: Path, target_dir: Path) -> None: - """Copy *source_dir* into *target_dir*, skipping excludes. - - Refuses to follow a symlink encountered mid-copy so a source swapped after - validation cannot smuggle external content into the staged package, and - re-enforces the depth, entry, and byte budgets. Entries count against the - entry budget as each listing is streamed, before it is sorted, so an - oversized source directory is rejected without being fully read. Iterative - so a deep tree cannot exhaust the Python recursion limit. - """ - remaining_bytes = _MAX_STEP_PACKAGE_BYTES - discovered_entries = 0 - pending = [(source_dir, target_dir, 0)] - while pending: - current, destination, depth = pending.pop() - if depth > _MAX_STEP_PACKAGE_DEPTH: - raise StepInstallError( - f"Step package exceeds the {_MAX_STEP_PACKAGE_DEPTH}-level " - "directory depth limit" - ) - try: - destination.mkdir(parents=True, exist_ok=True) - except OSError as exc: - raise StepInstallError( - f"Failed to stage step package: {exc}" - ) from exc - try: - entries = _scan_retained_entries( - current, _MAX_STEP_PACKAGE_FILES - discovered_entries - ) - except OSError as exc: - raise StepInstallError( - f"Failed to stage step package: {exc}" - ) from exc - discovered_entries += len(entries) - for entry in entries: - try: - mode = entry.stat(follow_symlinks=False).st_mode - except OSError as exc: - raise StepInstallError(f"Failed to stage step package: {exc}") from exc - target = destination / entry.name - if stat.S_ISLNK(mode): - raise StepInstallError( - f"Step package contains symlink: {entry.path}" - ) - if not (stat.S_ISDIR(mode) or stat.S_ISREG(mode)): - raise StepInstallError( - f"Step package contains unsupported file: {entry.path}" - ) - if stat.S_ISDIR(mode): - pending.append((Path(entry.path), target, depth + 1)) - else: - remaining_bytes -= _copy_regular_file( - entry.path, target, mode, remaining_bytes - ) +def _copy_package_tree(source_dir: Path, target_dir: Path) -> None: + """Copy *source_dir* into *target_dir*, skipping excludes (never follows symlinks).""" + _pi().copy_package_tree( + source_dir, + target_dir, + _step_spec(), + copy_file=lambda *args: _copy_regular_file(*args), + ) def _copy_regular_file( source: str, target: Path, expected_mode: int, remaining_bytes: int ) -> int: """Copy an inspected regular file within the remaining package byte budget.""" - flags = os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0) - try: - fd = os.open(source, flags) - except OSError as exc: - raise StepInstallError(f"Failed to stage step package: {exc}") from exc - try: - opened = os.fstat(fd) - source_state = os.stat(source, follow_symlinks=False) - if ( - not stat.S_ISREG(opened.st_mode) - or not stat.S_ISREG(source_state.st_mode) - or opened.st_dev != source_state.st_dev - or opened.st_ino != source_state.st_ino - or stat.S_IFMT(source_state.st_mode) != stat.S_IFMT(expected_mode) - ): - raise StepInstallError( - f"Step package file changed while staging: {source}" - ) - copied_bytes = 0 - with os.fdopen(fd, "rb", closefd=False) as source_file, target.open( - "xb" - ) as target_file: - while True: - chunk = source_file.read( - min(_COPY_CHUNK_BYTES, remaining_bytes - copied_bytes + 1) - ) - if not chunk: - break - if copied_bytes + len(chunk) > remaining_bytes: - raise StepInstallError( - f"Step package exceeds the {_MAX_STEP_PACKAGE_BYTES}-byte " - "total size limit while staging" - ) - target_file.write(chunk) - copied_bytes += len(chunk) - return copied_bytes - except OSError as exc: - raise StepInstallError(f"Failed to stage step package: {exc}") from exc - finally: - try: - os.close(fd) - except OSError: - pass + return _pi().copy_regular_file( + source, target, expected_mode, remaining_bytes, _step_spec() + ) def _replace_install( @@ -668,63 +403,9 @@ def _replace_install( force: bool, ) -> None: """Publish the staged package and record its registry entry.""" - from .catalog import StepValidationError - - if step_dir.exists(): - if not force: - raise StepInstallError( - f"Step directory already exists at '{step_dir}'. Remove it manually " - f"or use: specify workflow step remove {step_id}" - ) - # --force replacement: the replacement is fully staged and validated, - # so it is safe to remove the previous installation now. - try: - shutil.rmtree(step_dir) - except OSError as exc: - raise StepInstallError( - f"Failed to remove the existing step installation at " - f"'{step_dir}': {exc}. Reinstall from the original source with " - "--force." - ) from exc - try: - os.replace(staged_dir, step_dir) - except OSError as exc: - raise StepInstallError( - f"Failed to publish the replacement for step type '{step_id}': " - f"{exc}. The previous installation was removed; reinstall from " - "the original source with --force." - ) from exc - try: - registry.add(step_id, entry) - except (StepValidationError, OSError, TypeError, ValueError) as exc: - raise StepInstallError( - f"Failed to update the step registry for '{step_id}': {exc}. The " - "package was replaced but the registry metadata was not updated; " - "reinstall from the original source with --force." - ) from exc - return - - try: - os.replace(staged_dir, step_dir) - except OSError as exc: - raise StepInstallError( - f"Failed to install step '{step_id}': {exc}" - ) from exc - - try: - registry.add(step_id, entry) - except (StepValidationError, OSError, TypeError, ValueError) as exc: - # Fresh install: roll back the just-published directory so the system - # is not left with an unregistered step package on disk. - try: - shutil.rmtree(step_dir) - except OSError as cleanup_exc: - raise StepInstallError( - f"Failed to update the step registry for '{step_id}': {exc}. " - f"The unregistered package remains at '{step_dir}' because rollback " - f"failed: {cleanup_exc}. Remove it manually before reinstalling." - ) from exc - raise StepInstallError(str(exc)) from exc + _pi().replace_install( + step_dir, staged_dir, registry, step_id, entry, force=force, spec=_step_spec() + ) def install_step_package( @@ -742,111 +423,31 @@ def install_step_package( ``source`` is exactly ``"catalog"``, ``"local"``, or ``"url"``. Returns the registry entry that was persisted. """ - from .catalog import StepRegistry - - package_dir = Path(package_dir) - if package_dir.is_symlink(): - raise StepInstallError( - f"Refusing to install from a symlinked package directory: '{package_dir}'" - ) - if not package_dir.is_dir(): - raise StepInstallError(f"Step package directory not found: '{package_dir}'") - - validate_step_id(step_id) - steps_base_dir = resolve_steps_base_dir(project_root) - step_dir = _resolve_step_dir(steps_base_dir, step_id) - _reject_unsafe_destination(step_dir) - - # Reject a source that resolves to (or contains) the install destination: - # a --force replacement would otherwise delete the source before it can be - # copied. - try: - source_resolved = package_dir.resolve() - dest_resolved = step_dir.resolve() - except OSError as exc: - raise StepInstallError(f"Failed to resolve step package path: {exc}") from exc - if source_resolved == dest_resolved or dest_resolved.is_relative_to( - source_resolved - ): - raise StepInstallError( - f"Step package source resolves to the install destination: " - f"'{package_dir}'" - ) - - _reject_builtin_collision(step_id) - registry = StepRegistry(project_root) - _check_duplicate(registry, step_id, step_dir, force=force) - - # Validate source and all caller-controlled metadata before creating any - # project directories. The staged metadata is used for the final entry. - _build_entry( - step_id, - validate_step_package(package_dir, step_id), - source=source, - catalog_name=catalog_name, - catalog_metadata=catalog_metadata, - ) - - try: - steps_base_dir.mkdir(parents=True, exist_ok=True) - work_dir = Path( - tempfile.mkdtemp(prefix=_WORK_DIR_PREFIX, dir=steps_base_dir) - ) - except OSError as exc: - raise StepInstallError(f"Failed to create staging directory: {exc}") from exc - - staged_dir = work_dir / "staged" - committed = False - try: - _copy_package_tree(package_dir, staged_dir) - # Re-validate the complete staged copy: the source may have changed - # while it was copied. - staged_meta = validate_step_package(staged_dir, step_id) - entry = _build_entry( + # Every member is looked up when called so the established patch points on + # this module (``_copy_package_tree``, ``_replace_install``, + # ``_step_install_transaction``, ``validate_step_package``, ...) still apply. + shared = _pi() + rules = shared.PackageRules( + locate=lambda root, sid: _locate(root, sid), + open_registry=lambda root: _open_registry(root), + check_destination=lambda *a, **k: _check_destination(*a, **k), + validate=lambda path, sid: validate_step_package(path, sid), + build_entry=lambda step_meta: _build_entry( step_id, - staged_meta, + step_meta, source=source, catalog_name=catalog_name, catalog_metadata=catalog_metadata, - ) - # Serialize destination and registry changes. Source downloads/copying - # stay outside the lock, but all state that can conflict is reloaded and - # checked again immediately before publication. - with _step_install_transaction(project_root): - locked_base_dir = resolve_steps_base_dir(project_root) - if locked_base_dir != steps_base_dir: - raise StepInstallError( - "Step directory changed while staging; reinstall from the original source" - ) - step_dir = _resolve_step_dir(locked_base_dir, step_id) - _reject_unsafe_destination(step_dir) - _reject_builtin_collision(step_id) - registry = StepRegistry(project_root) - _check_duplicate(registry, step_id, step_dir, force=force) - _replace_install( - step_dir, - staged_dir, - registry, - step_id, - entry, - force=force, - ) - committed = True - finally: - primary_error = sys.exc_info()[1] - try: - shutil.rmtree(work_dir) - except OSError as cleanup_exc: - if work_dir.exists(): - message = ( - f"Could not remove step staging directory '{work_dir}': " - f"{cleanup_exc}" - ) - if primary_error is not None: - primary_error.add_note(message) - elif committed: - warnings.warn(message, UserWarning, stacklevel=2) - else: - raise StepInstallError(message) from cleanup_exc - - return entry + ), + ) + ops = shared.StagingOps( + copy=lambda src, dst: _copy_package_tree(src, dst), + publish=lambda *a, **k: _replace_install(*a, **k), + transaction=lambda root: _step_install_transaction(root), + ) + return shared.install_package( + shared.InstallRequest(project_root, step_id, Path(package_dir), force), + _step_spec(), + rules, + ops, + ) diff --git a/tests/specify_cli/workflows/step/test_installer_standalone.py b/tests/specify_cli/workflows/step/test_installer_standalone.py new file mode 100644 index 0000000000..1220091540 --- /dev/null +++ b/tests/specify_cli/workflows/step/test_installer_standalone.py @@ -0,0 +1,48 @@ +"""The step installer module must stay loadable outside the package. + +``.github/scripts/validate_community_workflow_step.py`` executes +``step/installer.py`` by file path to reuse its id validator and limits, so the +module may not import anything relative at import time. +""" + +from __future__ import annotations + +import importlib.util +import sys +from pathlib import Path + +import pytest + +INSTALLER = ( + Path(__file__).resolve().parents[4] + / "src" / "specify_cli" / "workflows" / "step" / "installer.py" +) + + +@pytest.fixture +def standalone(): + spec = importlib.util.spec_from_file_location("standalone_step_installer", INSTALLER) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +def test_module_loads_without_its_package(standalone): + assert "standalone_step_installer" not in sys.modules + assert standalone.StepInstallError.__name__ == "StepInstallError" + + +def test_validator_limits_and_excludes_are_available_standalone(standalone): + standalone.validate_step_id("my-step") + with pytest.raises(standalone.StepInstallError): + standalone.validate_step_id("../escape") + assert standalone._MAX_STEP_PACKAGE_FILES == 512 + assert standalone._MAX_STEP_PACKAGE_BYTES == 50 * 1024 * 1024 + assert standalone._MAX_STEP_PACKAGE_DEPTH == 32 + assert {".git", "__pycache__", ".DS_Store"} == set(standalone.EXCLUDE_NAMES) + + +def test_exclude_names_agree_with_the_shared_install_module(standalone): + from specify_cli.workflows import _package_install + + assert set(standalone.EXCLUDE_NAMES) == set(_package_install.EXCLUDE_NAMES) From 51e023bc08535970d4d7451d95254bcacc2860c5 Mon Sep 17 00:00:00 2001 From: Markus Date: Sat, 7 Nov 2026 08:39:24 +0100 Subject: [PATCH 03/11] feat(workflows): load project-installed workflow functions Add .specify/workflows/functions// packages (function.yml + __init__.py). The strict manifest parser, FunctionRegistry (a PackageRegistry subclass), install rules (export names checked against built-ins, the registry and package directories found on disk) and the loader are new; staging, copy, locking and replacement come from the shared package install. The loader clears the previous project's custom filters, validates every manifest and reports all collisions before importing any code, imports each package under a synthetic module name, and registers a package's exports all-or-nothing. run, resume, workflow add and workflow update load functions through one _load_project_workflow_plugins_or_exit helper that also renders OSError. "functions" becomes a reserved workflow id. Refs #5285 Assisted-by: pi coding agent (GitHub Copilot, model: claude-sonnet-5.5, autonomous) --- src/specify_cli/workflows/__init__.py | 7 + src/specify_cli/workflows/_commands.py | 22 +- src/specify_cli/workflows/command_add.py | 4 +- src/specify_cli/workflows/command_resume.py | 7 +- src/specify_cli/workflows/command_run.py | 5 +- src/specify_cli/workflows/command_update.py | 3 +- .../workflows/function/__init__.py | 38 ++ src/specify_cli/workflows/function/_domain.py | 145 +++++++ .../workflows/function/installer.py | 232 ++++++++++ src/specify_cli/workflows/function/loader.py | 144 ++++++ src/specify_cli/workflows/overlay/schema.py | 2 +- .../workflows/function/conftest.py | 12 +- .../specify_cli/workflows/function/helpers.py | 108 +++++ .../workflows/function/test_domain.py | 229 ++++++++++ .../workflows/function/test_loader.py | 410 ++++++++++++++++++ .../function/test_registry_parity.py | 172 ++++++++ .../workflows/function/test_reserved_ids.py | 60 +++ 17 files changed, 1581 insertions(+), 19 deletions(-) create mode 100644 src/specify_cli/workflows/function/__init__.py create mode 100644 src/specify_cli/workflows/function/_domain.py create mode 100644 src/specify_cli/workflows/function/installer.py create mode 100644 src/specify_cli/workflows/function/loader.py create mode 100644 tests/specify_cli/workflows/function/helpers.py create mode 100644 tests/specify_cli/workflows/function/test_domain.py create mode 100644 tests/specify_cli/workflows/function/test_loader.py create mode 100644 tests/specify_cli/workflows/function/test_registry_parity.py create mode 100644 tests/specify_cli/workflows/function/test_reserved_ids.py diff --git a/src/specify_cli/workflows/__init__.py b/src/specify_cli/workflows/__init__.py index 094d54a52f..06498f8aa2 100644 --- a/src/specify_cli/workflows/__init__.py +++ b/src/specify_cli/workflows/__init__.py @@ -243,3 +243,10 @@ def load_custom_steps(project_root: Path) -> list[str]: continue return loaded + + +def load_workflow_functions(project_root: Path) -> list[str]: + """Load project-installed workflow expression functions.""" + from .function.loader import load_workflow_functions as loader + + return loader(project_root) diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index 18a315528e..b4b2e09f6e 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -50,6 +50,26 @@ def _error_console(json_output: bool): return err_console if json_output else console +def _load_project_workflow_plugins_or_exit( + project_root: Path, *, json_output: bool = False, run_id: str | None = None +) -> None: + """Load project workflow steps and expression functions before validation.""" + from . import load_custom_steps + from .function import WorkflowFunctionError, load_workflow_functions + + try: + # Package imports may write diagnostics. Keep them off a JSON run's + # stdout, but render the expected failure after this context exits. + with _stdout_to_stderr_when(json_output): + load_custom_steps(project_root) + load_workflow_functions(project_root) + except (WorkflowFunctionError, OSError) as exc: + # OSError (an unreadable steps or functions directory or package file) + # was rendered by run/resume's integration-load handler before plugin + # loading moved ahead of it; keep that behavior for every entry point. + _fail_integration_load(exc, json_output=json_output, run_id=run_id) + + def _print_package_error(exc: Exception) -> None: """Render a package install/remove failure: retained notes as warnings, then the error.""" for note in getattr(exc, "__notes__", ()): @@ -214,7 +234,7 @@ def _resolve_installed_workflow_ownership( _WORKFLOW_ID_PATTERN = re.compile(r"^[a-z0-9](?:[a-z0-9-]*[a-z0-9])?$") -_RESERVED_WORKFLOW_IDS: frozenset[str] = frozenset({"overlays", "runs", "steps"}) +_RESERVED_WORKFLOW_IDS: frozenset[str] = frozenset({"overlays", "runs", "steps", "functions"}) def _reject_insecure_download_redirect(old_url: str, new_url: str) -> None: diff --git a/src/specify_cli/workflows/command_add.py b/src/specify_cli/workflows/command_add.py index 4d1cfb6567..37f03433b2 100644 --- a/src/specify_cli/workflows/command_add.py +++ b/src/specify_cli/workflows/command_add.py @@ -34,8 +34,6 @@ def _prepare_workflow_add( project_root: cli.Path, source: str, *, dev: bool, from_url: str | None, version: str | None, selected_catalog: bool = False, ) -> cli.Path: - from . import load_custom_steps - if version is not None and ( dev or from_url is not None or ( not selected_catalog and ( @@ -50,7 +48,7 @@ def _prepare_workflow_add( raise cli.typer.Exit(1) if version is not None or selected_catalog: cli._validate_workflow_id_or_exit(source) - load_custom_steps(project_root) + cli._load_project_workflow_plugins_or_exit(project_root) cli._open_workflow_registry(project_root) workflows_dir = project_root / ".specify" / "workflows" if from_url is not None and not dev: diff --git a/src/specify_cli/workflows/command_resume.py b/src/specify_cli/workflows/command_resume.py index 8e75fdc71b..9cf2e1b0ed 100644 --- a/src/specify_cli/workflows/command_resume.py +++ b/src/specify_cli/workflows/command_resume.py @@ -20,15 +20,16 @@ def workflow_resume( ): """Resume a paused or failed workflow run.""" from ..integrations.installer import IntegrationInstallError - from . import load_custom_steps from ._execution import CheckpointError from .engine import RunState, WorkflowEngine project_root = cli._require_specify_project(load_integrations=False) + cli._load_project_workflow_plugins_or_exit( + project_root, json_output=json_output, run_id=run_id + ) try: + engine = WorkflowEngine(project_root) with cli._stdout_to_stderr_when(json_output): - load_custom_steps(project_root) - engine = WorkflowEngine(project_root) engine._load_integrations() except (IntegrationInstallError, OSError) as exc: cli._fail_integration_load(exc, json_output=json_output, run_id=run_id) diff --git a/src/specify_cli/workflows/command_run.py b/src/specify_cli/workflows/command_run.py index 157edb964b..9269d400e8 100644 --- a/src/specify_cli/workflows/command_run.py +++ b/src/specify_cli/workflows/command_run.py @@ -20,7 +20,6 @@ def workflow_run( ): """Run a workflow from an installed ID or local YAML path.""" from ..integrations.installer import IntegrationInstallError - from . import load_custom_steps from .engine import WorkflowEngine source_path = cli.Path(source).expanduser() @@ -48,10 +47,10 @@ def workflow_run( else: project_root = cli._require_specify_project(load_integrations=False) + cli._load_project_workflow_plugins_or_exit(project_root, json_output=json_output) try: + engine = WorkflowEngine(project_root) with cli._stdout_to_stderr_when(json_output): - load_custom_steps(project_root) - engine = WorkflowEngine(project_root) engine._load_integrations() except (IntegrationInstallError, OSError) as exc: cli._fail_integration_load(exc, json_output=json_output) diff --git a/src/specify_cli/workflows/command_update.py b/src/specify_cli/workflows/command_update.py index e95ef3c151..f4b6c2c481 100644 --- a/src/specify_cli/workflows/command_update.py +++ b/src/specify_cli/workflows/command_update.py @@ -14,11 +14,10 @@ def workflow_update( """Update installed workflow(s) to the latest catalog version.""" from packaging import version as pkg_version - from . import load_custom_steps from .catalog import WorkflowCatalog, WorkflowCatalogError project_root = cli._require_specify_project() - load_custom_steps(project_root) + cli._load_project_workflow_plugins_or_exit(project_root) registry = cli._open_workflow_registry(project_root) workflows_dir = project_root / ".specify" / "workflows" cli._reject_unsafe_dir(project_root / ".specify", ".specify") diff --git a/src/specify_cli/workflows/function/__init__.py b/src/specify_cli/workflows/function/__init__.py new file mode 100644 index 0000000000..eb44045ffc --- /dev/null +++ b/src/specify_cli/workflows/function/__init__.py @@ -0,0 +1,38 @@ +"""Workflow function domain and ``specify workflow function`` commands.""" + +from __future__ import annotations + +import typer + +function_app = typer.Typer( + name="function", help="Manage workflow expression functions", add_completion=False +) + + +def register(app: typer.Typer) -> None: + from . import command_add # noqa: F401 + from . import command_list # noqa: F401 + from . import command_remove # noqa: F401 + + app.add_typer(function_app, name="function") + + +from ._domain import ( # noqa: E402 + FunctionExport, + FunctionManifest, + FunctionRegistry, + WorkflowFunctionError, + parse_function_manifest, +) +from .loader import load_workflow_functions # noqa: E402 + +__all__ = [ + "FunctionExport", + "FunctionManifest", + "FunctionRegistry", + "WorkflowFunctionError", + "function_app", + "load_workflow_functions", + "parse_function_manifest", + "register", +] diff --git a/src/specify_cli/workflows/function/_domain.py b/src/specify_cli/workflows/function/_domain.py new file mode 100644 index 0000000000..2d38322dfc --- /dev/null +++ b/src/specify_cli/workflows/function/_domain.py @@ -0,0 +1,145 @@ +"""Domain models and persistence for installed workflow functions.""" + +from __future__ import annotations + +import re +from dataclasses import dataclass +from pathlib import Path +from typing import Any + +import yaml + +from .._package_registry import PackageRegistry +from ..expressions.filters import format_filter_usage +from ..step.installer import StepInstallError, validate_step_id + + +class WorkflowFunctionError(ValueError): + """A user-facing workflow function manifest, loader, or install error.""" + + +@dataclass(frozen=True) +class FunctionExport: + name: str + handler: str + min_args: int = 0 + max_args: int = 0 + description: str = "" + + @property + def usage(self) -> str: + return format_filter_usage(self.name, self.min_args, self.max_args) + + +@dataclass(frozen=True) +class FunctionManifest: + id: str + name: str = "" + version: str = "0.0.0" + description: str = "" + exports: tuple[FunctionExport, ...] = () + + +_RESERVED_FUNCTION_IDS = frozenset({".cache", "function-registry.json"}) +_FUNCTION_NAME_RE = re.compile(r"^[a-z_][a-z0-9_]*$") +_HANDLER_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") + + +def _validate_function_id(value: str) -> None: + try: + validate_step_id(value) + except StepInstallError as exc: + raise WorkflowFunctionError(str(exc).replace("step id", "function id")) from exc + if value in _RESERVED_FUNCTION_IDS: + raise WorkflowFunctionError(f"Invalid function id '{value}': reserved name") + + +def _string(data: dict[str, Any], key: str, default: str = "") -> str: + value = data.get(key, default) + if not isinstance(value, str): + raise WorkflowFunctionError(f"function.yml '{key}' must be a string") + return value + + +def parse_function_manifest(text: str, expected_id: str | None = None) -> FunctionManifest: + """Parse the strict ``function.yml`` schema without importing code.""" + try: + raw = yaml.safe_load(text) + except yaml.YAMLError as exc: + raise WorkflowFunctionError(f"Invalid function.yml: {exc}") from exc + if not isinstance(raw, dict): + raise WorkflowFunctionError("function.yml must be a YAML mapping") + allowed = {"id", "name", "version", "description", "functions"} + unknown = sorted(set(raw) - allowed) + if unknown: + raise WorkflowFunctionError(f"function.yml contains unknown key(s): {', '.join(unknown)}") + function_id = raw.get("id") + if not isinstance(function_id, str) or not function_id: + raise WorkflowFunctionError("function.yml is missing required 'id'") + _validate_function_id(function_id) + if expected_id is not None and function_id != expected_id: + raise WorkflowFunctionError( + f"function.yml id ({function_id!r}) does not match function ID ({expected_id!r})" + ) + functions = raw.get("functions") + if not isinstance(functions, list) or not functions: + raise WorkflowFunctionError("function.yml 'functions' must be a non-empty list") + exports: list[FunctionExport] = [] + seen: set[str] = set() + for index, item in enumerate(functions): + if not isinstance(item, dict): + raise WorkflowFunctionError(f"function.yml function at index {index} must be a mapping") + allowed_export = {"name", "handler", "min_args", "max_args", "description"} + unknown_export = sorted(set(item) - allowed_export) + if unknown_export: + raise WorkflowFunctionError( + f"function.yml function at index {index} contains unknown key(s): {', '.join(unknown_export)}" + ) + name = item.get("name") + handler = item.get("handler") + if not isinstance(name, str) or _FUNCTION_NAME_RE.fullmatch(name) is None: + raise WorkflowFunctionError( + f"function.yml function at index {index} has invalid name {name!r}" + ) + if name in seen: + raise WorkflowFunctionError(f"function.yml declares function '{name}' more than once") + seen.add(name) + if not isinstance(handler, str) or _HANDLER_RE.fullmatch(handler) is None: + raise WorkflowFunctionError( + f"function.yml function '{name}' has invalid handler {handler!r}" + ) + minimum = item.get("min_args", 0) + maximum = item.get("max_args", 0) + if ( + isinstance(minimum, bool) or isinstance(maximum, bool) + or not isinstance(minimum, int) or not isinstance(maximum, int) + or minimum < 0 or maximum < minimum + ): + raise WorkflowFunctionError( + f"function.yml function '{name}' has invalid argument bounds" + ) + description = item.get("description", "") + if not isinstance(description, str): + raise WorkflowFunctionError(f"function.yml function '{name}' description must be a string") + exports.append(FunctionExport(name, handler, minimum, maximum, description)) + return FunctionManifest( + function_id, + _string(raw, "name"), + _string(raw, "version", "0.0.0"), + _string(raw, "description"), + tuple(exports), + ) + + +class FunctionRegistry(PackageRegistry): + """Persistent metadata for packages under ``workflows/functions``.""" + + REGISTRY_FILE = "function-registry.json" + RECORDS_KEY = "functions" + DIRECTORY = "functions" + LABEL = "function" + ERROR = WorkflowFunctionError + + @property + def functions_dir(self) -> Path: + return self.base_dir diff --git a/src/specify_cli/workflows/function/installer.py b/src/specify_cli/workflows/function/installer.py new file mode 100644 index 0000000000..307046ebef --- /dev/null +++ b/src/specify_cli/workflows/function/installer.py @@ -0,0 +1,232 @@ +"""Function-specific installation rules for workflow function packages. + +The staged install flow, copy, locking and replacement are shared with step +packages in :mod:`specify_cli.workflows._package_install`. This module supplies +only what is specific to functions: the ``function.yml`` manifest, the export +name checks (D4) and the registry metadata. Installing never imports the +package. +""" + +from __future__ import annotations + +import contextlib +from pathlib import Path +from typing import Any + +from .. import _package_install as _pi +from .._package_install import ( + InstallRequest, + PackageLimits, + PackageLocation, + PackageRules, + PackageSpec, +) +from ._domain import ( + _RESERVED_FUNCTION_IDS, + FunctionManifest, + FunctionRegistry, + WorkflowFunctionError, + _validate_function_id, + parse_function_manifest, +) + +_MAX_FUNCTION_PACKAGE_FILES = 512 +_MAX_FUNCTION_PACKAGE_BYTES = 50 * 1024 * 1024 +_MAX_FUNCTION_PACKAGE_DEPTH = 32 + +_SOURCES = frozenset({"local", "url"}) + + +def _function_spec() -> PackageSpec: + """Build the function spec from the module limits at call time.""" + return PackageSpec( + noun="Function", + kind_label="Function package", + error=WorkflowFunctionError, + limits=PackageLimits( + _MAX_FUNCTION_PACKAGE_FILES, + _MAX_FUNCTION_PACKAGE_BYTES, + _MAX_FUNCTION_PACKAGE_DEPTH, + ), + ) + + +@contextlib.contextmanager +def _function_install_transaction(project_root: Path): + """Serialize function directory and registry mutations (install and remove).""" + with _pi.install_transaction(project_root, _function_spec()): + yield + + +def _resolve_functions_dir(project_root: Path) -> Path: + """Resolve ``.specify/workflows/functions`` refusing symlinked parents.""" + return _pi.resolve_base_dir(project_root, _function_spec()) + + +def validate_function_package(package_dir: Path, function_id: str) -> FunctionManifest: + """Validate a materialized package without importing its Python module.""" + package_dir = Path(package_dir) + _pi.check_package_shape(package_dir, _function_spec(), ("function.yml", "__init__.py")) + try: + text = (package_dir / "function.yml").read_text(encoding="utf-8") + except (OSError, UnicodeError) as exc: + raise WorkflowFunctionError(f"Invalid function.yml: {exc}") from exc + return parse_function_manifest(text, function_id) + + +def resolve_package_root(extracted_root: Path) -> Path: + """Resolve a root-level or single-nested function package directory.""" + return _pi.resolve_package_root(extracted_root, _function_spec(), "function.yml") + + +def _locate(project_root: Path, function_id: str) -> PackageLocation: + spec = _function_spec() + _validate_function_id(function_id) + base_dir = _resolve_functions_dir(project_root) + destination = _pi.resolve_destination(base_dir, function_id, spec) + _pi.reject_unsafe_destination(destination, spec) + return PackageLocation(base_dir, destination) + + +def _check_destination( + registry: FunctionRegistry, + function_id: str, + location: PackageLocation, + *, + force: bool, +) -> None: + _pi.check_duplicate( + registry, function_id, location.destination, force=force, spec=_function_spec() + ) + + +def _claimed_names(registry: FunctionRegistry, own_id: str) -> dict[str, tuple[str, bool]]: + """Map each function name to ``(package id, found only on disk)``. + + The registry is not the whole truth: the loader imports every package + directory it finds, so a hand-copied package or one whose registry entry was + lost still claims its names. Directories the loader would skip (symlinks, + missing files, invalid manifests) claim nothing. *own_id* is the package + being installed or replaced and never conflicts with itself. + """ + claimed: dict[str, tuple[str, bool]] = {} + for package_id, metadata in registry.list().items(): + if package_id == own_id or not isinstance(metadata, dict): + continue + for item in metadata.get("functions", []): + name = item.get("name") if isinstance(item, dict) else item + if isinstance(name, str): + claimed.setdefault(name, (package_id, False)) + + base_dir = registry.base_dir + try: + package_dirs = sorted(base_dir.iterdir(), key=lambda path: path.name) + except OSError: + return claimed + for package_dir in package_dirs: + if package_dir.name == own_id or package_dir.name in _RESERVED_FUNCTION_IDS: + continue + manifest_path = package_dir / "function.yml" + init_path = package_dir / "__init__.py" + if ( + package_dir.is_symlink() + or not package_dir.is_dir() + or manifest_path.is_symlink() + or init_path.is_symlink() + or not manifest_path.is_file() + or not init_path.is_file() + ): + continue + try: + manifest = parse_function_manifest( + manifest_path.read_text(encoding="utf-8"), package_dir.name + ) + except (OSError, UnicodeError, WorkflowFunctionError): + continue + for export in manifest.exports: + claimed.setdefault(export.name, (package_dir.name, True)) + return claimed + + +def _check_exports( + registry: FunctionRegistry, + function_id: str, + manifest: FunctionManifest, + *, + force: bool, +) -> None: + """Reject exports that shadow a built-in or a package already installed (D4).""" + from ..expressions.filters import BUILTIN_FILTERS + + claimed = _claimed_names(registry, manifest.id) + for export in manifest.exports: + if export.name in BUILTIN_FILTERS: + raise WorkflowFunctionError( + f"Workflow function '{export.name}' conflicts with a built-in filter" + ) + if export.name in claimed: + package_id, on_disk_only = claimed[export.name] + where = " (found on disk, not in the registry)" if on_disk_only else "" + raise WorkflowFunctionError( + f"Workflow function '{export.name}' is already provided by " + f"package '{package_id}'{where}" + ) + + +def check_installable(project_root: Path, function_id: str, *, force: bool = False) -> Path: + """Advisory preflight used by the CLI before any download.""" + location = _locate(project_root, function_id) + _check_destination(FunctionRegistry(project_root), function_id, location, force=force) + return location.destination + + +def _metadata(manifest: FunctionManifest, source: str) -> dict[str, Any]: + if source not in _SOURCES: + raise WorkflowFunctionError("Function install source must be 'local' or 'url'") + return { + "name": manifest.name or manifest.id, + "version": manifest.version, + "description": manifest.description, + "source": source, + "functions": [ + { + "name": export.name, + "handler": export.handler, + "min_args": export.min_args, + "max_args": export.max_args, + "description": export.description, + } + for export in manifest.exports + ], + } + + +def install_function_package( + project_root: Path, + function_id: str, + package_dir: Path, + *, + source: str, + force: bool = False, +) -> dict[str, Any]: + """Validate, copy, and register a function package without importing it.""" + rules = PackageRules( + locate=_locate, + open_registry=FunctionRegistry, + check_destination=_check_destination, + validate=lambda path, fid: validate_function_package(path, fid), + build_entry=lambda manifest: _metadata(manifest, source), + check_contents=lambda registry, fid, manifest, *, force: _check_exports( + registry, fid, manifest, force=force + ), + ) + return _pi.install_package( + InstallRequest(project_root, function_id, Path(package_dir), force), + _function_spec(), + rules, + _pi.StagingOps( + copy=lambda src, dst: _pi.copy_package_tree(src, dst, _function_spec()), + publish=lambda *a, **k: _pi.replace_install(*a, spec=_function_spec(), **k), + transaction=lambda root: _function_install_transaction(root), + ), + ) diff --git a/src/specify_cli/workflows/function/loader.py b/src/specify_cli/workflows/function/loader.py new file mode 100644 index 0000000000..454e922204 --- /dev/null +++ b/src/specify_cli/workflows/function/loader.py @@ -0,0 +1,144 @@ +"""Load project-installed workflow function packages.""" + +from __future__ import annotations + +import hashlib +import re +from pathlib import Path +from typing import Any + +from .. import _drop_modules, _import_package +from ..expressions.filters import ( + BUILTIN_FILTERS, + register_custom_filter, + unregister_custom_filter, + unregister_custom_filters, +) +from ._domain import FunctionManifest, WorkflowFunctionError, parse_function_manifest + +_CUSTOM_FUNCTION_MODULES: set[str] = set() + + +def _unload_custom_functions() -> None: + unregister_custom_filters() + _drop_modules(_CUSTOM_FUNCTION_MODULES) + _CUSTOM_FUNCTION_MODULES.clear() + + +def _function_paths(project_root: Path) -> tuple[Path, Path]: + root = Path(project_root) + current = root + for part in (".specify", "workflows", "functions"): + current /= part + if current.is_symlink(): + raise WorkflowFunctionError(f"Refusing to load symlinked function directory '{current}'") + return current, root + + +def _collision_error(collisions: list[tuple[str, str, str]]) -> WorkflowFunctionError: + """Describe every collision as ``(name, existing owner, new owner)``.""" + if len(collisions) == 1: + name, other, source = collisions[0] + return WorkflowFunctionError( + f"Workflow function '{name}' collides between {other} and {source}; " + "rename or remove the package." + ) + details = "; ".join( + f"'{name}' between {other} and {source}" for name, other, source in collisions + ) + return WorkflowFunctionError( + f"Workflow functions collide: {details}; rename or remove the packages." + ) + + +def load_workflow_functions(project_root: Path) -> list[str]: + """Load every valid function package for *project_root*. + + Discovery and validation happen before importing any package, so a manifest + collision cannot execute package code. A package is registered atomically: + a bad export does not leave its earlier exports or synthetic modules behind. + """ + _unload_custom_functions() + functions_dir, _ = _function_paths(Path(project_root)) + if not functions_dir.exists(): + return [] + if not functions_dir.is_dir(): + raise WorkflowFunctionError(f"Function directory is not a directory: '{functions_dir}'") + + manifests: list[tuple[Path, FunctionManifest]] = [] + owners: dict[str, str] = {name: "built-in filters" for name in BUILTIN_FILTERS} + collisions: list[tuple[str, str, str]] = [] + for package_dir in sorted(functions_dir.iterdir(), key=lambda p: p.name): + if package_dir.name in {"function-registry.json", ".cache"}: + continue + if package_dir.is_symlink(): + continue + if not package_dir.is_dir(): + continue + manifest_path = package_dir / "function.yml" + init_path = package_dir / "__init__.py" + if manifest_path.is_symlink() or init_path.is_symlink(): + continue + if not manifest_path.is_file() or not init_path.is_file(): + continue + try: + manifest = parse_function_manifest( + manifest_path.read_text(encoding="utf-8"), package_dir.name + ) + except (OSError, UnicodeError): + continue + except WorkflowFunctionError: + # Runtime loading follows custom-step behavior for malformed or + # broken packages: ignore that package, but never hide collisions + # among manifests that were successfully parsed. + continue + for export in manifest.exports: + if export.name in owners: + collisions.append((export.name, owners[export.name], f"package '{manifest.id}'")) + else: + owners[export.name] = f"package '{manifest.id}'" + manifests.append((package_dir, manifest)) + if collisions: + raise _collision_error(collisions) + + loaded: list[str] = [] + for package_dir, manifest in manifests: + safe_id = re.sub(r"[^A-Za-z0-9_]", "_", manifest.id) + module_name = ( + f"_speckit_workflow_function_{safe_id}_" + f"{hashlib.sha256(str(package_dir).encode()).hexdigest()[:8]}" + ) + added: list[str] = [] + successful = False + try: + module = _import_package(module_name, package_dir) + handlers: list[tuple[Any, Any]] = [] + for export in manifest.exports: + handler = getattr(module, export.handler, None) + if not callable(handler): + raise WorkflowFunctionError( + f"Workflow function '{export.name}' handler '{export.handler}' " + f"in package '{manifest.id}' is not callable" + ) + handlers.append((export, handler)) + for export, handler in handlers: + register_custom_filter( + export.name, handler, export.min_args, export.max_args + ) + added.append(export.name) + _CUSTOM_FUNCTION_MODULES.add(module_name) + loaded.extend(export.name for export in manifest.exports) + successful = True + except WorkflowFunctionError: + # A package is an all-or-nothing registration unit. Broken package + # code follows the installed-step loader's skip behavior; only the + # collision pre-pass above is intentionally fatal. + pass + except Exception: + pass + finally: + if not successful: + for name in added: + unregister_custom_filter(name) + _drop_modules({module_name}) + return loaded diff --git a/src/specify_cli/workflows/overlay/schema.py b/src/specify_cli/workflows/overlay/schema.py index 969bbd94a9..7337ae6932 100644 --- a/src/specify_cli/workflows/overlay/schema.py +++ b/src/specify_cli/workflows/overlay/schema.py @@ -11,7 +11,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"}) +_RESERVED_WORKFLOW_IDS: frozenset[str] = frozenset({"overlays", "runs", "steps", "functions"}) VALID_OPERATIONS = frozenset({"insert_after", "insert_before", "replace", "remove"}) diff --git a/tests/specify_cli/workflows/function/conftest.py b/tests/specify_cli/workflows/function/conftest.py index 7bbbe9182e..f6b17c3523 100644 --- a/tests/specify_cli/workflows/function/conftest.py +++ b/tests/specify_cli/workflows/function/conftest.py @@ -1,7 +1,7 @@ """Fixtures for workflow function tests. -The custom-filter table is process-global, so every test starts and ends with -only built-in filters registered. +The custom-filter table and synthetic modules are process-global, so every +test starts and ends with only built-in filters registered. """ from __future__ import annotations @@ -10,9 +10,9 @@ @pytest.fixture(autouse=True) -def _reset_custom_filters(): - from specify_cli.workflows.expressions import unregister_custom_filters +def _reset_custom_functions(): + from specify_cli.workflows.function.loader import _unload_custom_functions - unregister_custom_filters() + _unload_custom_functions() yield - unregister_custom_filters() + _unload_custom_functions() diff --git a/tests/specify_cli/workflows/function/helpers.py b/tests/specify_cli/workflows/function/helpers.py new file mode 100644 index 0000000000..e8eb204c9e --- /dev/null +++ b/tests/specify_cli/workflows/function/helpers.py @@ -0,0 +1,108 @@ +"""Shared builders for workflow function tests.""" + +from __future__ import annotations + +import io +import threading +import zipfile +from collections.abc import Callable, Iterator +from contextlib import contextmanager +from http.server import BaseHTTPRequestHandler, HTTPServer +from pathlib import Path +from typing import Any + +import yaml + +TRIM_CODE = "def trim(value):\n return value.strip()\n" + + +def trim_export(**overrides: Any) -> dict[str, Any]: + return {"name": "trim", "handler": "trim", **overrides} + + +def write_package( + package_dir: Path, + function_id: str, + functions: list[dict[str, Any]] | None = None, + code: str = TRIM_CODE, + **manifest: Any, +) -> Path: + """Write a function package (``function.yml`` + ``__init__.py``).""" + package_dir.mkdir(parents=True, exist_ok=True) + data = {"id": function_id, "functions": functions or [trim_export()], **manifest} + (package_dir / "function.yml").write_text(yaml.safe_dump(data), encoding="utf-8") + (package_dir / "__init__.py").write_text(code, encoding="utf-8") + return package_dir + + +def functions_dir(project_root: Path) -> Path: + return project_root / ".specify" / "workflows" / "functions" + + +def install_dir(project_root: Path, function_id: str) -> Path: + return functions_dir(project_root) / function_id + + +def write_installed( + project_root: Path, + function_id: str, + functions: list[dict[str, Any]] | None = None, + code: str = TRIM_CODE, + **manifest: Any, +) -> Path: + """Place a package directly under the project (as a hand copy would).""" + return write_package( + install_dir(project_root, function_id), function_id, functions, code, **manifest + ) + + +def zip_package(package_dir: Path, *, top_level: str | None = None) -> bytes: + buffer = io.BytesIO() + with zipfile.ZipFile(buffer, "w") as archive: + for path in sorted(package_dir.rglob("*")): + if path.is_file(): + name = path.relative_to(package_dir).as_posix() + archive.write(path, f"{top_level}/{name}" if top_level else name) + return buffer.getvalue() + + +@contextmanager +def serve_files(files: dict[str, bytes]) -> Iterator[Callable[[str], str]]: + """Serve *files* from a loopback HTTP server. + + Yields a name -> URL function whose ``requests`` attribute lists the paths + requested so far. + """ + + requests: list[str] = [] + + class Handler(BaseHTTPRequestHandler): + def do_GET(self): # noqa: N802 + requests.append(self.path) + body = files.get(self.path.lstrip("/")) + if body is None: + self.send_response(404) + self.end_headers() + return + self.send_response(200) + self.send_header("Content-Type", "application/zip") + self.send_header("Content-Length", str(len(body))) + self.end_headers() + self.wfile.write(body) + + def log_message(self, *args): + pass + + server = HTTPServer(("127.0.0.1", 0), Handler) + thread = threading.Thread(target=server.serve_forever, daemon=True) + thread.start() + try: + def url(name: str) -> str: + return f"http://127.0.0.1:{server.server_port}/{name}" + + url.requests = requests # type: ignore[attr-defined] + yield url + finally: + server.shutdown() + server.server_close() + thread.join(timeout=5) diff --git a/tests/specify_cli/workflows/function/test_domain.py b/tests/specify_cli/workflows/function/test_domain.py new file mode 100644 index 0000000000..9a8cd21bbc --- /dev/null +++ b/tests/specify_cli/workflows/function/test_domain.py @@ -0,0 +1,229 @@ +"""Manifest parser, id validation and persistent registry.""" + +from __future__ import annotations + +import json +import os + +import pytest +import yaml + +from specify_cli.workflows.function import ( + FunctionRegistry, + WorkflowFunctionError, + parse_function_manifest, +) +from specify_cli.workflows.function._domain import _validate_function_id +from specify_cli.workflows.step.installer import StepInstallError, validate_step_id + +from .helpers import functions_dir + + +def manifest(**overrides): + data = {"id": "text", "functions": [{"name": "trim", "handler": "trim"}]} + data.update(overrides) + return yaml.safe_dump(data) + + +class TestManifestParsing: + def test_minimal_manifest_uses_defaults(self): + parsed = parse_function_manifest(manifest(), "text") + assert (parsed.id, parsed.name, parsed.version, parsed.description) == ( + "text", "", "0.0.0", "" + ) + (export,) = parsed.exports + assert (export.name, export.handler, export.min_args, export.max_args) == ( + "trim", "trim", 0, 0 + ) + assert export.description == "" and export.usage == "trim" + + def test_full_manifest(self): + parsed = parse_function_manifest( + manifest( + name="Text helpers", version="1.2.3", description="d", + functions=[ + {"name": "pad", "handler": "pad_text", "min_args": 2, + "max_args": 3, "description": "Pad it"}, + {"name": "trim", "handler": "trim"}, + ], + ), + "text", + ) + assert (parsed.name, parsed.version, parsed.description) == ("Text helpers", "1.2.3", "d") + assert [e.name for e in parsed.exports] == ["pad", "trim"] + assert parsed.exports[0].usage == "pad(arg1, arg2, arg3)" + assert parsed.exports[0].description == "Pad it" + + def test_expected_id_is_optional(self): + assert parse_function_manifest(manifest()).id == "text" + + @pytest.mark.parametrize( + ("text", "message"), + [ + ("a: [", "Invalid function.yml"), + ("- a\n- b\n", "must be a YAML mapping"), + ("", "must be a YAML mapping"), + (manifest(extra=1), "unknown key"), + (yaml.safe_dump({"functions": [{"name": "a", "handler": "a"}]}), "missing required 'id'"), + (manifest(id=""), "missing required 'id'"), + (manifest(id=5), "missing required 'id'"), + (manifest(functions=None), "non-empty list"), + (manifest(functions=[]), "non-empty list"), + (manifest(functions={"name": "a"}), "non-empty list"), + (manifest(functions=["a"]), "must be a mapping"), + (manifest(functions=[{"name": "a", "handler": "a", "x": 1}]), "unknown key"), + (manifest(functions=[{"handler": "a"}]), "invalid name"), + (manifest(functions=[{"name": "a"}]), "invalid handler"), + (manifest(name=3), "'name' must be a string"), + (manifest(version=1.0), "'version' must be a string"), + (manifest(description=[]), "'description' must be a string"), + (manifest(functions=[{"name": "a", "handler": "a", "description": 1}]), "description must be a string"), + ], + ) + def test_invalid_manifests_are_rejected(self, text, message): + with pytest.raises(WorkflowFunctionError, match=message): + parse_function_manifest(text, "text") + + def test_id_must_match_expected_id(self): + with pytest.raises(WorkflowFunctionError, match="does not match"): + parse_function_manifest(manifest(), "other") + + @pytest.mark.parametrize("name", ["Trim", "a-b", "1a", "a.b", "a b", "", None, 3]) + def test_invalid_function_names(self, name): + data = manifest(functions=[{"name": name, "handler": "h"}]) + with pytest.raises(WorkflowFunctionError, match="invalid name"): + parse_function_manifest(data, "text") + + @pytest.mark.parametrize("handler", ["a.b", "1a", "a b", "a-b", "", None, 3, "a()"]) + def test_invalid_handlers(self, handler): + data = manifest(functions=[{"name": "a", "handler": handler}]) + with pytest.raises(WorkflowFunctionError, match="invalid handler"): + parse_function_manifest(data, "text") + + def test_duplicate_name_in_one_manifest(self): + data = manifest(functions=[ + {"name": "a", "handler": "x"}, {"name": "a", "handler": "y"}, + ]) + with pytest.raises(WorkflowFunctionError, match="more than once"): + parse_function_manifest(data, "text") + + @pytest.mark.parametrize( + ("minimum", "maximum"), + [(2, 1), (-1, 0), (0, -1), (True, 1), (0, True), ("1", 2), (0, 1.5), (None, 1)], + ) + def test_invalid_bounds(self, minimum, maximum): + data = manifest(functions=[ + {"name": "a", "handler": "a", "min_args": minimum, "max_args": maximum} + ]) + with pytest.raises(WorkflowFunctionError, match="argument bounds"): + parse_function_manifest(data, "text") + + def test_max_defaults_to_zero_so_min_alone_is_invalid(self): + data = manifest(functions=[{"name": "a", "handler": "a", "min_args": 1}]) + with pytest.raises(WorkflowFunctionError, match="argument bounds"): + parse_function_manifest(data, "text") + + def test_unsafe_yaml_tags_are_not_executed(self): + with pytest.raises(WorkflowFunctionError, match="Invalid function.yml"): + parse_function_manifest("id: !!python/object/apply:os.system ['true']", "x") + + +class TestFunctionIds: + @pytest.mark.parametrize("value", ["text", "my-fn", "a1", "MyFn", "a_b"]) + def test_valid_ids(self, value): + _validate_function_id(value) + + @pytest.mark.parametrize( + "value", + ["", " ", " a", "a ", "a/b", "a\\b", ".", "..", ".hidden", "a.", "../x", + "con", "NUL", "a:b", "a\x01"], + ) + def test_ids_rejected_by_the_shared_step_validator_are_rejected(self, value): + with pytest.raises(StepInstallError): + validate_step_id(value) + with pytest.raises(WorkflowFunctionError, match="function id"): + _validate_function_id(value) + + @pytest.mark.parametrize("value", [".cache", "function-registry.json"]) + def test_function_specific_reserved_names(self, value): + with pytest.raises(WorkflowFunctionError): + _validate_function_id(value) + + +class TestRegistry: + def test_empty_registry(self, project_dir): + registry = FunctionRegistry(project_dir) + assert registry.list() == {} and not registry.is_installed("x") + assert registry.get("x") is None + + def test_add_persists_and_reloads(self, project_dir): + registry = FunctionRegistry(project_dir) + registry.add("text", {"name": "Text", "functions": []}) + assert FunctionRegistry(project_dir).get("text")["name"] == "Text" + stored = json.loads((functions_dir(project_dir) / "function-registry.json").read_text()) + assert stored["schema_version"] == "1.0" + assert {"installed_at", "updated_at"} <= set(stored["functions"]["text"]) + + def test_replace_keeps_installed_at(self, project_dir): + registry = FunctionRegistry(project_dir) + registry.add("text", {"name": "one"}) + first = registry.get("text")["installed_at"] + registry.add("text", {"name": "two"}) + entry = registry.get("text") + assert entry["name"] == "two" and entry["installed_at"] == first + + def test_remove(self, project_dir): + registry = FunctionRegistry(project_dir) + registry.add("text", {}) + assert registry.remove("text") is True + assert registry.remove("text") is False + assert FunctionRegistry(project_dir).list() == {} + + def test_list_returns_a_copy(self, project_dir): + registry = FunctionRegistry(project_dir) + registry.add("text", {}) + registry.list().clear() + assert registry.is_installed("text") + + @pytest.mark.parametrize("content", ["{bad", "[]", '{"functions": []}', "", "\x00"]) + def test_corrupt_registry_loads_as_empty(self, project_dir, content): + path = functions_dir(project_dir) / "function-registry.json" + path.parent.mkdir(parents=True) + path.write_text(content, encoding="utf-8") + assert FunctionRegistry(project_dir).list() == {} + + def test_failed_save_rolls_back_in_memory_state(self, project_dir, monkeypatch): + registry = FunctionRegistry(project_dir) + registry.add("keep", {"name": "keep"}) + monkeypatch.setattr( + registry, "save", + lambda: (_ for _ in ()).throw(WorkflowFunctionError("disk full")), + ) + with pytest.raises(WorkflowFunctionError, match="disk full"): + registry.add("new", {}) + with pytest.raises(WorkflowFunctionError, match="disk full"): + registry.add("keep", {"name": "changed"}) + with pytest.raises(WorkflowFunctionError, match="disk full"): + registry.remove("keep") + assert registry.list().keys() == {"keep"} + assert registry.get("keep")["name"] == "keep" + + def test_save_leaves_no_temp_files(self, project_dir): + FunctionRegistry(project_dir).add("text", {}) + assert [p.name for p in functions_dir(project_dir).iterdir()] == ["function-registry.json"] + + @pytest.mark.skipif(not hasattr(os, "symlink"), reason="symlinks are unavailable") + def test_symlinked_functions_dir_is_not_read_or_written(self, project_dir): + outside = project_dir.parent / f"{project_dir.name}-outside" + outside.mkdir() + (outside / "function-registry.json").write_text( + json.dumps({"functions": {"evil": {}}}), encoding="utf-8" + ) + functions_dir(project_dir).symlink_to(outside, target_is_directory=True) + registry = FunctionRegistry(project_dir) + assert registry.list() == {} + with pytest.raises(WorkflowFunctionError, match="symlink"): + registry.add("text", {}) + assert json.loads((outside / "function-registry.json").read_text()) == { + "functions": {"evil": {}} + } diff --git a/tests/specify_cli/workflows/function/test_loader.py b/tests/specify_cli/workflows/function/test_loader.py new file mode 100644 index 0000000000..3b5ec07cf8 --- /dev/null +++ b/tests/specify_cli/workflows/function/test_loader.py @@ -0,0 +1,410 @@ +"""Loader: discovery, import isolation, atomic registration, collisions.""" + +from __future__ import annotations + +import os +import shutil +import sys + +import pytest + +from specify_cli.workflows.expressions import ( + BUILTIN_FILTERS, + FILTER_TABLE, + evaluate_expression, +) +from specify_cli.workflows.base import StepContext +from specify_cli.workflows.function import WorkflowFunctionError, load_workflow_functions +from specify_cli.workflows.function import loader as loader_module + +from .helpers import functions_dir, install_dir, trim_export, write_installed + +PREFIX = "_speckit_workflow_function_" + + +def synthetic_modules(): + return [name for name in sys.modules if name.startswith(PREFIX)] + + +def custom_filters(): + return sorted(set(FILTER_TABLE) - set(BUILTIN_FILTERS)) + + +def run(template, **inputs): + return evaluate_expression(template, StepContext(inputs=inputs)) + + +class TestLoading: + def test_no_functions_directory(self, project_dir): + assert load_workflow_functions(project_dir) == [] + assert custom_filters() == [] and synthetic_modules() == [] + + def test_loads_declared_callables_without_a_base_class(self, project_dir): + write_installed( + project_dir, "text", + [trim_export(), {"name": "shout", "handler": "loud", "min_args": 0, "max_args": 1}], + code=( + "def trim(v):\n return v.strip()\n" + "def loud(v, suffix='!'):\n return v.upper() + suffix\n" + "def undeclared(v):\n return 'never'\n" + ), + ) + assert load_workflow_functions(project_dir) == ["trim", "shout"] + assert custom_filters() == ["shout", "trim"] + assert run("{{ inputs.s | trim | shout('?') }}", s=" a ") == "A?" + + def test_handlers_may_be_any_callable(self, project_dir): + write_installed( + project_dir, "text", + code="class Trim:\n def __call__(self, v):\n return v.strip()\n" + "trim = Trim()\n", + ) + assert load_workflow_functions(project_dir) == ["trim"] + assert run("{{ inputs.s | trim }}", s=" a ") == "a" + + def test_relative_imports_work(self, project_dir): + package = write_installed( + project_dir, "text", code="from .helpers import clean as trim\n" + ) + (package / "helpers.py").write_text( + "def clean(v):\n return v.strip()\n", encoding="utf-8" + ) + assert load_workflow_functions(project_dir) == ["trim"] + assert run("{{ inputs.s | trim }}", s=" a ") == "a" + + def test_packages_with_same_internal_module_names_are_isolated(self, project_dir): + for package_id, name, marker in (("one", "f1", "1"), ("two", "f2", "2")): + package = write_installed( + project_dir, package_id, + [{"name": name, "handler": "f"}], + code="from .helpers import f\n", + ) + (package / "helpers.py").write_text( + f"def f(v):\n return {marker!r}\n", encoding="utf-8" + ) + assert sorted(load_workflow_functions(project_dir)) == ["f1", "f2"] + assert run("{{ inputs.s | f1 }}{{ inputs.s | f2 }}", s="x") == "12" + + def test_modules_use_synthetic_names_and_not_plain_imports(self, project_dir): + write_installed(project_dir, "text") + load_workflow_functions(project_dir) + (name,) = synthetic_modules() + assert name.startswith(f"{PREFIX}text_") + assert "text" not in sys.modules + + def test_non_package_entries_are_ignored(self, project_dir): + write_installed(project_dir, "text") + (functions_dir(project_dir) / "function-registry.json").write_text("{}") + (functions_dir(project_dir) / ".cache").mkdir() + (functions_dir(project_dir) / "stray.txt").write_text("x") + (functions_dir(project_dir) / "empty").mkdir() + assert load_workflow_functions(project_dir) == ["trim"] + + def test_reload_for_another_project_replaces_functions_and_modules(self, tmp_path): + project_a, project_b = tmp_path / "a", tmp_path / "b" + write_installed(project_a, "text", code="def trim(v):\n return 'a'\n") + write_installed( + project_b, "other", [{"name": "shout", "handler": "shout"}], + code="def shout(v):\n return 'b'\n", + ) + assert load_workflow_functions(project_a) == ["trim"] + assert run("{{ inputs.s | trim }}", s="") == "a" + before = synthetic_modules() + + assert load_workflow_functions(project_b) == ["shout"] + assert custom_filters() == ["shout"] + assert not set(before) & set(synthetic_modules()) + with pytest.raises(ValueError, match="unknown filter 'trim'"): + run("{{ inputs.s | trim }}", s="") + + def test_reload_after_removal_forgets_the_package(self, project_dir): + write_installed(project_dir, "text") + assert load_workflow_functions(project_dir) == ["trim"] + shutil.rmtree(install_dir(project_dir, "text")) + assert load_workflow_functions(project_dir) == [] + assert custom_filters() == [] and synthetic_modules() == [] + + def test_reload_picks_up_changed_code(self, project_dir): + write_installed(project_dir, "text", code="def trim(v):\n return 'old'\n") + load_workflow_functions(project_dir) + write_installed(project_dir, "text", code="def trim(v):\n return 'changed'\n") + load_workflow_functions(project_dir) + assert run("{{ inputs.s | trim }}", s="") == "changed" + + def test_reload_same_project_is_idempotent(self, project_dir): + write_installed(project_dir, "text") + assert load_workflow_functions(project_dir) == ["trim"] + assert load_workflow_functions(project_dir) == ["trim"] + assert len(synthetic_modules()) == 1 + + +class TestBrokenPackages: + """A broken package is skipped; healthy packages still load.""" + + def healthy(self, project_dir): + write_installed(project_dir, "good", [{"name": "ok", "handler": "ok"}], + code="def ok(v):\n return 'ok'\n") + + def assert_only_healthy_loaded(self, project_dir): + assert load_workflow_functions(project_dir) == ["ok"] + assert custom_filters() == ["ok"] + assert len(synthetic_modules()) == 1 and "good" in synthetic_modules()[0] + + def test_invalid_manifest(self, project_dir): + self.healthy(project_dir) + package = write_installed(project_dir, "bad") + (package / "function.yml").write_text("id: bad\nfunctions: nope\n") + self.assert_only_healthy_loaded(project_dir) + + def test_unreadable_manifest(self, project_dir): + self.healthy(project_dir) + package = write_installed(project_dir, "bad") + (package / "function.yml").write_bytes(b"\xff\xfe\x00") + self.assert_only_healthy_loaded(project_dir) + + def test_id_must_equal_directory_name(self, project_dir): + self.healthy(project_dir) + write_installed(project_dir, "bad", id="other") + self.assert_only_healthy_loaded(project_dir) + + def test_missing_manifest_or_init(self, project_dir): + self.healthy(project_dir) + (write_installed(project_dir, "a") / "function.yml").unlink() + (write_installed(project_dir, "b") / "__init__.py").unlink() + self.assert_only_healthy_loaded(project_dir) + + def test_missing_handler(self, project_dir): + self.healthy(project_dir) + write_installed(project_dir, "bad", [{"name": "nope", "handler": "absent"}]) + self.assert_only_healthy_loaded(project_dir) + + def test_non_callable_handler(self, project_dir): + self.healthy(project_dir) + write_installed(project_dir, "bad", code="trim = 5\n") + self.assert_only_healthy_loaded(project_dir) + + def test_import_failure_leaves_no_modules(self, project_dir): + self.healthy(project_dir) + package = write_installed(project_dir, "bad", code="from .helpers import x\n") + (package / "helpers.py").write_text("raise RuntimeError('boom')\n") + self.assert_only_healthy_loaded(project_dir) + + def test_import_failure_after_submodule_import_leaves_no_submodules(self, project_dir): + self.healthy(project_dir) + package = write_installed( + project_dir, "bad", code="from . import helpers\nraise RuntimeError('late')\n" + ) + (package / "helpers.py").write_text("X = 1\n") + self.assert_only_healthy_loaded(project_dir) + assert not [n for n in sys.modules if n.startswith(PREFIX) and "bad" in n] + + def test_syntax_error(self, project_dir): + self.healthy(project_dir) + write_installed(project_dir, "bad", code="def trim(:\n") + self.assert_only_healthy_loaded(project_dir) + + def test_bad_second_handler_registers_nothing_from_that_package(self, project_dir): + self.healthy(project_dir) + write_installed( + project_dir, "bad", + [{"name": "first", "handler": "first"}, {"name": "second", "handler": "absent"}], + code="def first(v):\n return v\n", + ) + self.assert_only_healthy_loaded(project_dir) + + def test_registration_failure_rolls_back_the_whole_package(self, project_dir, monkeypatch): + write_installed(project_dir, "a_good", [{"name": "ok", "handler": "ok"}], + code="def ok(v):\n return 'ok'\n") + write_installed( + project_dir, "b_bad", + [{"name": "first", "handler": "f"}, {"name": "second", "handler": "f"}], + code="def f(v):\n return v\n", + ) + write_installed(project_dir, "c_good", [{"name": "ok2", "handler": "ok"}], + code="def ok(v):\n return 'ok2'\n") + real = loader_module.register_custom_filter + + def flaky(name, *args): + if name == "second": + raise RuntimeError("injected") + real(name, *args) + + monkeypatch.setattr(loader_module, "register_custom_filter", flaky) + assert load_workflow_functions(project_dir) == ["ok", "ok2"] + assert custom_filters() == ["ok", "ok2"] + assert not [n for n in synthetic_modules() if "b_bad" in n] + + +class TestCollisions: + """Collisions are fatal and detected before any package code runs.""" + + def marker_code(self, project_dir, name): + marker = project_dir / f"{name}.imported" + return marker, f"open({str(marker)!r}, 'w').close()\ndef trim(v):\n return v\n" + + def assert_clean(self): + assert custom_filters() == [] and synthetic_modules() == [] + + def test_package_shadowing_a_builtin(self, project_dir): + marker, code = self.marker_code(project_dir, "shadow") + write_installed(project_dir, "text", [{"name": "upper", "handler": "trim"}], code=code) + with pytest.raises(WorkflowFunctionError) as info: + load_workflow_functions(project_dir) + message = str(info.value) + assert "'upper'" in message and "built-in filters" in message + assert "package 'text'" in message and "rename or remove" in message + assert not marker.exists() + self.assert_clean() + assert FILTER_TABLE["upper"] is BUILTIN_FILTERS["upper"] + + def test_two_packages_providing_the_same_name(self, project_dir): + marker_a, code_a = self.marker_code(project_dir, "a") + marker_b, code_b = self.marker_code(project_dir, "b") + write_installed(project_dir, "alpha", code=code_a) + write_installed(project_dir, "beta", code=code_b) + with pytest.raises(WorkflowFunctionError) as info: + load_workflow_functions(project_dir) + message = str(info.value) + assert "'trim'" in message and "package 'alpha'" in message and "package 'beta'" in message + assert not marker_a.exists() and not marker_b.exists() + self.assert_clean() + + def test_every_builtin_shadow_in_one_package_is_reported(self, project_dir): + marker, code = self.marker_code(project_dir, "multi") + write_installed( + project_dir, "text", + [{"name": "upper", "handler": "trim"}, {"name": "lower", "handler": "trim"}, + {"name": "fine", "handler": "trim"}], + code=code, + ) + with pytest.raises(WorkflowFunctionError) as info: + load_workflow_functions(project_dir) + message = str(info.value) + assert message == ( + "Workflow functions collide: 'upper' between built-in filters and package " + "'text'; 'lower' between built-in filters and package 'text'; " + "rename or remove the packages." + ) + assert "'fine'" not in message and not marker.exists() + self.assert_clean() + + def test_every_cross_package_clash_is_reported(self, project_dir): + write_installed( + project_dir, "alpha", + [{"name": "trim", "handler": "trim"}, {"name": "pad", "handler": "trim"}], + ) + write_installed( + project_dir, "beta", + [{"name": "pad", "handler": "trim"}, {"name": "trim", "handler": "trim"}], + ) + with pytest.raises(WorkflowFunctionError) as info: + load_workflow_functions(project_dir) + message = str(info.value) + assert "'pad' between package 'alpha' and package 'beta'" in message + assert "'trim' between package 'alpha' and package 'beta'" in message + self.assert_clean() + + def test_mixed_builtin_and_cross_package_clashes_are_reported_together(self, project_dir): + write_installed(project_dir, "alpha") + write_installed( + project_dir, "beta", + [{"name": "trim", "handler": "trim"}, {"name": "length", "handler": "trim"}], + ) + with pytest.raises(WorkflowFunctionError) as info: + load_workflow_functions(project_dir) + message = str(info.value) + assert "'trim' between package 'alpha' and package 'beta'" in message + assert "'length' between built-in filters and package 'beta'" in message + + def test_three_packages_claiming_one_name_report_both_later_packages(self, project_dir): + for package_id in ("alpha", "beta", "gamma"): + write_installed(project_dir, package_id) + with pytest.raises(WorkflowFunctionError) as info: + load_workflow_functions(project_dir) + message = str(info.value) + assert "between package 'alpha' and package 'beta'" in message + assert "between package 'alpha' and package 'gamma'" in message + + def test_message_order_is_deterministic(self, project_dir): + write_installed(project_dir, "zeta", [{"name": "upper", "handler": "trim"}]) + write_installed(project_dir, "alpha", [{"name": "lower", "handler": "trim"}]) + messages = set() + for _ in range(3): + with pytest.raises(WorkflowFunctionError) as info: + load_workflow_functions(project_dir) + messages.add(str(info.value)) + (message,) = messages + assert message.index("package 'alpha'") < message.index("package 'zeta'") + + def test_single_collision_keeps_the_singular_message(self, project_dir): + write_installed(project_dir, "text", [{"name": "upper", "handler": "trim"}]) + with pytest.raises(WorkflowFunctionError) as info: + load_workflow_functions(project_dir) + assert str(info.value) == ( + "Workflow function 'upper' collides between built-in filters and package " + "'text'; rename or remove the package." + ) + + def test_collision_wins_over_unrelated_broken_packages(self, project_dir): + write_installed(project_dir, "alpha") + write_installed(project_dir, "beta") + write_installed(project_dir, "gamma", [{"name": "x", "handler": "absent"}]) + with pytest.raises(WorkflowFunctionError, match="collides"): + load_workflow_functions(project_dir) + + def test_previous_project_state_is_cleared_when_the_next_load_collides(self, tmp_path): + good, bad = tmp_path / "good", tmp_path / "bad" + write_installed(good, "text") + write_installed(bad, "text", [{"name": "upper", "handler": "trim"}]) + load_workflow_functions(good) + with pytest.raises(WorkflowFunctionError): + load_workflow_functions(bad) + self.assert_clean() + + def test_invalid_manifest_does_not_claim_names(self, project_dir): + write_installed(project_dir, "alpha") + package = write_installed(project_dir, "beta") + (package / "function.yml").write_text("id: beta\nfunctions: [bad") + assert load_workflow_functions(project_dir) == ["trim"] + + +class TestFilesystemSafety: + pytestmark = pytest.mark.skipif(not hasattr(os, "symlink"), reason="symlinks are unavailable") + + @pytest.mark.parametrize("link", [".specify", ".specify/workflows", ".specify/workflows/functions"]) + def test_symlinked_parent_is_refused(self, project_dir, tmp_path, link): + outside = tmp_path / "outside" + write_installed(outside, "text") + target = project_dir / link + if target.exists(): + if target.is_dir(): + shutil.rmtree(target) + else: + target.unlink() + if link.endswith("functions"): + outside_dir = outside / ".specify" / "workflows" / "functions" + else: + outside_dir = outside / ".specify" if link == ".specify" else outside / ".specify" / "workflows" + target.symlink_to(outside_dir, target_is_directory=True) + with pytest.raises(WorkflowFunctionError, match="symlinked"): + load_workflow_functions(project_dir) + assert custom_filters() == [] and synthetic_modules() == [] + + def test_symlinked_package_directory_is_skipped(self, project_dir, tmp_path): + real = write_installed(tmp_path / "elsewhere", "text") + (functions_dir(project_dir)).mkdir(parents=True, exist_ok=True) + install_dir(project_dir, "text").symlink_to(real, target_is_directory=True) + assert load_workflow_functions(project_dir) == [] + + @pytest.mark.parametrize("name", ["function.yml", "__init__.py"]) + def test_symlinked_package_files_are_skipped(self, project_dir, tmp_path, name): + package = write_installed(project_dir, "text") + real = tmp_path / "real" + shutil.move(package / name, real) + (package / name).symlink_to(real) + assert load_workflow_functions(project_dir) == [] + + def test_functions_path_that_is_a_file_is_an_error(self, project_dir): + functions_dir(project_dir).write_text("not a directory") + with pytest.raises(WorkflowFunctionError, match="not a directory"): + load_workflow_functions(project_dir) diff --git a/tests/specify_cli/workflows/function/test_registry_parity.py b/tests/specify_cli/workflows/function/test_registry_parity.py new file mode 100644 index 0000000000..6aa320d1b2 --- /dev/null +++ b/tests/specify_cli/workflows/function/test_registry_parity.py @@ -0,0 +1,172 @@ +"""Step and function registries share one persistence implementation. + +Every case runs against both kinds, so a behavior can only exist for both. +""" + +from __future__ import annotations + +import json +import os +import stat + +import pytest + +from specify_cli.workflows._package_registry import PackageRegistry +from specify_cli.workflows.function import FunctionRegistry, WorkflowFunctionError +from specify_cli.workflows.step.catalog import StepRegistry, StepValidationError + +KINDS = { + "step": (StepRegistry, StepValidationError, "steps", "step-registry.json"), + "function": (FunctionRegistry, WorkflowFunctionError, "functions", "function-registry.json"), +} + + +@pytest.fixture(params=sorted(KINDS)) +def kind(request, project_dir): + cls, error, directory, filename = KINDS[request.param] + base = project_dir / ".specify" / "workflows" / directory + return type("Kind", (), { + "cls": cls, "error": error, "key": directory, "path": base / filename, + "base": base, "root": project_dir, "label": request.param, + }) + + +def test_both_registries_derive_from_the_shared_base(): + assert issubclass(StepRegistry, PackageRegistry) + assert issubclass(FunctionRegistry, PackageRegistry) + assert StepRegistry.save is PackageRegistry.save + assert FunctionRegistry.add is PackageRegistry.add + assert StepRegistry.remove is FunctionRegistry.remove + + +def test_each_kind_keeps_its_established_attributes(kind): + registry = kind.cls(kind.root) + assert registry.registry_path == kind.path + assert registry.data == {"schema_version": "1.0", kind.key: {}} + assert getattr(registry, f"{kind.key}_dir") == kind.base + + +def test_add_get_list_remove_roundtrip(kind): + registry = kind.cls(kind.root) + registry.add("one", {"name": "One"}) + assert kind.cls(kind.root).get("one")["name"] == "One" + assert list(kind.cls(kind.root).list()) == ["one"] and registry.is_installed("one") + assert registry.remove("one") is True and registry.remove("one") is False + assert kind.cls(kind.root).list() == {} + + +def test_add_stamps_times_and_preserves_installed_at(kind): + registry = kind.cls(kind.root) + registry.add("one", {"name": "a"}) + first = registry.get("one")["installed_at"] + registry.add("one", {"name": "b"}) + entry = registry.get("one") + assert entry["installed_at"] == first and entry["name"] == "b" and "updated_at" in entry + + +def test_metadata_is_copied_not_aliased(kind): + registry = kind.cls(kind.root) + metadata = {"functions": [{"name": "x"}]} + registry.add("one", metadata) + metadata["functions"][0]["name"] = "mutated" + assert registry.get("one")["functions"][0]["name"] == "x" + + +def test_non_dict_existing_entry_is_replaced(kind): + kind.base.mkdir(parents=True) + kind.path.write_text(json.dumps({kind.key: {"one": "junk"}})) + registry = kind.cls(kind.root) + registry.add("one", {"name": "ok"}) + assert registry.get("one")["name"] == "ok" + + +@pytest.mark.parametrize("content", ["{bad", "[]", "", "null", "\x00"]) +def test_unreadable_registry_loads_as_empty(kind, content): + kind.base.mkdir(parents=True) + kind.path.write_text(content) + assert kind.cls(kind.root).list() == {} + + +def test_missing_records_key_is_repaired_and_other_keys_kept(kind): + kind.base.mkdir(parents=True) + kind.path.write_text(json.dumps({"schema_version": "1.0", "extra": 1, kind.key: []})) + registry = kind.cls(kind.root) + assert registry.list() == {} and registry.data["extra"] == 1 + + +def test_failed_save_rolls_back_every_mutation(kind, monkeypatch): + registry = kind.cls(kind.root) + registry.add("keep", {"name": "keep"}) + + def fail(): + raise kind.error("disk full") + + monkeypatch.setattr(registry, "save", fail) + with pytest.raises(kind.error, match="disk full"): + registry.add("new", {}) + with pytest.raises(kind.error, match="disk full"): + registry.add("keep", {"name": "changed"}) + with pytest.raises(kind.error, match="disk full"): + registry.remove("keep") + assert list(registry.list()) == ["keep"] and registry.get("keep")["name"] == "keep" + + +def test_unserializable_metadata_is_rejected_and_rolled_back(kind): + registry = kind.cls(kind.root) + with pytest.raises((TypeError, ValueError, kind.error)): + registry.add("bad", {"value": object()}) + assert not registry.is_installed("bad") + assert not kind.path.exists() or "bad" not in json.loads(kind.path.read_text())[kind.key] + + +def test_write_error_uses_the_kind_error_type_and_label(kind, monkeypatch): + registry = kind.cls(kind.root) + + def boom(*args, **kwargs): + raise OSError("no space") + + monkeypatch.setattr(os, "replace", boom) + with pytest.raises(kind.error) as info: + registry.add("one", {}) + assert f"Failed to write {kind.label} registry" in str(info.value) + assert [p.name for p in kind.base.iterdir()] == [] # no temp file left behind + + +def test_save_leaves_only_the_registry_file(kind): + kind.cls(kind.root).add("one", {}) + assert [p.name for p in kind.base.iterdir()] == [kind.path.name] + + +@pytest.mark.skipif(not hasattr(os, "chmod"), reason="chmod is unavailable") +def test_save_preserves_existing_file_mode(kind): + registry = kind.cls(kind.root) + registry.add("one", {}) + os.chmod(kind.path, 0o640) + registry.add("two", {}) + assert stat.S_IMODE(kind.path.stat().st_mode) == 0o640 + + +@pytest.mark.skipif(not hasattr(os, "symlink"), reason="symlinks are unavailable") +class TestSymlinks: + def test_symlinked_directory_is_neither_read_nor_written(self, kind, tmp_path): + outside = tmp_path / "outside" + outside.mkdir() + (outside / kind.path.name).write_text(json.dumps({kind.key: {"evil": {}}})) + kind.base.parent.mkdir(parents=True, exist_ok=True) + kind.base.symlink_to(outside, target_is_directory=True) + registry = kind.cls(kind.root) + assert registry.list() == {} + with pytest.raises(kind.error, match="symlinked path"): + registry.add("one", {}) + assert json.loads((outside / kind.path.name).read_text()) == {kind.key: {"evil": {}}} + + def test_symlinked_registry_file_is_neither_read_nor_written(self, kind, tmp_path): + outside = tmp_path / "outside.json" + outside.write_text(json.dumps({kind.key: {"evil": {}}})) + kind.base.mkdir(parents=True) + kind.path.symlink_to(outside) + registry = kind.cls(kind.root) + assert registry.list() == {} + with pytest.raises(kind.error, match="symlinked path"): + registry.add("one", {}) + assert json.loads(outside.read_text()) == {kind.key: {"evil": {}}} diff --git a/tests/specify_cli/workflows/function/test_reserved_ids.py b/tests/specify_cli/workflows/function/test_reserved_ids.py new file mode 100644 index 0000000000..7edf404b34 --- /dev/null +++ b/tests/specify_cli/workflows/function/test_reserved_ids.py @@ -0,0 +1,60 @@ +"""``functions`` is reserved so a workflow cannot occupy the package directory.""" + +from __future__ import annotations + +import pytest +import yaml +from typer.testing import CliRunner + +from specify_cli import app +from specify_cli.workflows import _commands +from specify_cli.workflows.overlay import schema +from specify_cli.workflows.overlay.schema import validate_overlay_yaml + + +def test_reserved_sets_agree_and_include_functions(): + assert "functions" in _commands._RESERVED_WORKFLOW_IDS + assert "functions" in schema._RESERVED_WORKFLOW_IDS + assert _commands._RESERVED_WORKFLOW_IDS == schema._RESERVED_WORKFLOW_IDS + assert {"overlays", "runs", "steps"} <= schema._RESERVED_WORKFLOW_IDS + + +def test_workflow_add_rejects_the_id(project_dir, monkeypatch): + source = project_dir / "incoming.yml" + source.write_text( + yaml.safe_dump({ + "schema_version": "1.0", + "workflow": {"id": "functions", "name": "Bad", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "shell", "run": "echo hi"}], + }), + encoding="utf-8", + ) + monkeypatch.chdir(project_dir) + result = CliRunner().invoke(app, ["workflow", "add", str(source)]) + assert result.exit_code != 0 + assert not (project_dir / ".specify" / "workflows" / "functions").exists() + + +def test_safe_workflow_id_dir_rejects_the_id(project_dir): + import typer + + workflows_dir = project_dir / ".specify" / "workflows" + with pytest.raises(typer.Exit): + _commands._safe_workflow_id_dir(workflows_dir, "functions") + assert not (workflows_dir / "functions").exists() + + +def test_overlay_cannot_extend_the_reserved_id(): + _, errors = validate_overlay_yaml({ + "id": "ov", "extends": "functions", "priority": 10, + "edits": [{"operation": "remove", "anchor": "a"}], + }) + assert any("extends" in error for error in errors) + + +def test_overlay_can_still_extend_an_ordinary_id(): + _, errors = validate_overlay_yaml({ + "id": "ov", "extends": "wf", "priority": 10, + "edits": [{"operation": "remove", "anchor": "a"}], + }) + assert not any("extends" in error for error in errors) From 66eabe2ead7c3392300899a10d555238480cff00 Mon Sep 17 00:00:00 2001 From: Markus Date: Sat, 7 Nov 2026 08:39:24 +0100 Subject: [PATCH 04/11] feat(workflows): add the specify workflow function commands specify workflow function add (--dev or --from archive URL, --force), list (registry metadata only, --json) and remove. add and remove use the shared trust prompt, download checks, project lock and rollback; list never imports package code. The three commands are registered in the MCP inventory as currently unavailable. Refs #5285 Assisted-by: pi coding agent (GitHub Copilot, model: claude-sonnet-5.5, autonomous) --- src/specify_cli/workflows/_commands.py | 2 + src/specify_cli/workflows/_mcp.py | 38 +- .../workflows/function/command_add.py | 60 ++ .../workflows/function/command_list.py | 71 +++ .../workflows/function/command_remove.py | 38 ++ .../workflows/function/test_command_add.py | 290 +++++++++ .../workflows/function/test_command_list.py | 138 ++++ .../workflows/function/test_command_remove.py | 174 +++++ .../workflows/function/test_installer.py | 592 ++++++++++++++++++ .../workflows/function/test_wiring.py | 269 ++++++++ 10 files changed, 1670 insertions(+), 2 deletions(-) create mode 100644 src/specify_cli/workflows/function/command_add.py create mode 100644 src/specify_cli/workflows/function/command_list.py create mode 100644 src/specify_cli/workflows/function/command_remove.py create mode 100644 tests/specify_cli/workflows/function/test_command_add.py create mode 100644 tests/specify_cli/workflows/function/test_command_list.py create mode 100644 tests/specify_cli/workflows/function/test_command_remove.py create mode 100644 tests/specify_cli/workflows/function/test_installer.py create mode 100644 tests/specify_cli/workflows/function/test_wiring.py diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index b4b2e09f6e..3411cc5142 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -1835,11 +1835,13 @@ def workflow_step_remove(*args, **kwargs): def register(app: typer.Typer) -> None: """Attach the workflow command group to the root Typer app.""" from .catalog import register as register_catalog + from .function import register as register_function from .overlay import register as register_overlay from .step import register as register_step register_catalog(workflow_app) register_step(workflow_app) + register_function(workflow_app) register_overlay(workflow_app) # isort: off diff --git a/src/specify_cli/workflows/_mcp.py b/src/specify_cli/workflows/_mcp.py index a6b6278393..402b64f643 100644 --- a/src/specify_cli/workflows/_mcp.py +++ b/src/specify_cli/workflows/_mcp.py @@ -7,8 +7,9 @@ * Project resolution does not load integrations by default (``specify_cli._require_specify_project``). -* Run/resume explicitly load integrations and custom steps before execution. - Add/update also import custom steps while validating workflow definitions. +* Run/resume explicitly load integrations, custom steps and workflow functions + before execution. Add/update also import these workflow plugins while + validating workflow definitions. * Catalog search/info can write project caches. Authenticated HTTP can invoke ``az account get-access-token`` through the configured Azure CLI provider, so even discovery/download paths may require execution capability. @@ -302,6 +303,39 @@ class WorkflowToolInventory: capabilities=_LOCAL_WRITE, network_access="none", ), + WorkflowToolInventory( + operation_id="workflow.function.add", + cli_path="specify workflow function add", + mcp_tool_name="specify_workflow_function_add", + disposition_reason=( + "Function package source/trust selection and executable-code installation remain " + "CLI-owned; no typed function-add operation or MCP adapter." + ), + capabilities=_WRITE_AND_EXECUTION, + network_access="optional", + ), + WorkflowToolInventory( + operation_id="workflow.function.list", + cli_path="specify workflow function list", + mcp_tool_name="specify_workflow_function_list", + disposition_reason=( + "Function metadata listing is currently CLI-rendered; no shared typed list " + "operation or MCP adapter." + ), + capabilities=_LOCAL_READ, + network_access="none", + ), + WorkflowToolInventory( + operation_id="workflow.function.remove", + cli_path="specify workflow function remove", + mcp_tool_name="specify_workflow_function_remove", + disposition_reason=( + "Function package removal and registry mutation remain CLI-owned; no typed " + "function-remove operation or MCP adapter." + ), + capabilities=_LOCAL_WRITE, + network_access="none", + ), WorkflowToolInventory( operation_id="workflow.step.add", cli_path="specify workflow step add", diff --git a/src/specify_cli/workflows/function/command_add.py b/src/specify_cli/workflows/function/command_add.py new file mode 100644 index 0000000000..47261345d5 --- /dev/null +++ b/src/specify_cli/workflows/function/command_add.py @@ -0,0 +1,60 @@ +"""CLI for installing workflow function packages.""" + +from __future__ import annotations + +from pathlib import Path +from typing import Annotated + +from .. import _archive_install as archive +from .. import _commands as cli +from . import function_app +from . import installer + + +def _install_from_url(project_root: Path, function_id: str, url: str, force: bool) -> dict: + """Install a function package archive from a direct URL.""" + return archive.install_from_archive_url( + archive.ArchiveRequest(project_root, function_id, url, force), + installer._function_spec(), + archive.ArchiveHooks( + preflight=lambda root, fid, force: installer.check_installable( + root, fid, force=force + ), + resolve_root=lambda extracted: installer.resolve_package_root(extracted), + install=lambda root, fid, package, force: installer.install_function_package( + root, fid, package, source="url", force=force + ), + ), + ) + + +@function_app.command("add") +def workflow_function_add( + function_id: str = cli.typer.Argument(..., help="Function package ID"), + dev: Annotated[str | None, cli.typer.Option("--dev", help="Install from a local package directory")] = None, + from_url: Annotated[str | None, cli.typer.Option("--from", help="Install from an archive URL")] = None, + force: bool = cli.typer.Option(False, "--force", help="Replace an existing installation"), +): + """Install project workflow expression functions.""" + project_root = cli._require_specify_project() + if (dev is None) == (from_url is None): + cli.console.print("[red]Error:[/red] exactly one of --dev or --from is required") + raise cli.typer.Exit(1) + try: + if dev is not None: + source = Path(dev).expanduser() + if source.is_symlink() or not source.is_dir(): + raise installer.WorkflowFunctionError( + f"--dev source must be a directory: '{source}'" + ) + installer.install_function_package( + project_root, function_id, source, source="local", force=force + ) + else: + _install_from_url(project_root, function_id, from_url or "", force) + except installer.WorkflowFunctionError as exc: + cli._print_package_error(exc) + raise cli.typer.Exit(1) from exc + cli.console.print( + f"[green]✓[/green] Workflow function package '{cli._escape_markup(function_id)}' installed" + ) diff --git a/src/specify_cli/workflows/function/command_list.py b/src/specify_cli/workflows/function/command_list.py new file mode 100644 index 0000000000..fd985bfe81 --- /dev/null +++ b/src/specify_cli/workflows/function/command_list.py @@ -0,0 +1,71 @@ +"""CLI for listing installed workflow function metadata.""" + +from __future__ import annotations + +import json + +from .. import _commands as cli +from . import function_app +from ._domain import FunctionRegistry +from ..expressions.filters import format_filter_usage + + +@function_app.command("list") +def workflow_function_list( + json_output: bool = cli.typer.Option(False, "--json", help="Emit machine-readable JSON") +): + """List installed workflow functions without importing package code.""" + project_root = cli._require_specify_project() + registry = FunctionRegistry(project_root) + installed = registry.list() + rows = [] + for function_id in sorted(installed): + metadata = installed[function_id] + if not isinstance(metadata, dict): + continue + functions = [] + for item in metadata.get("functions", []): + if not isinstance(item, dict): + continue + name = item.get("name", "") + minimum = item.get("min_args", 0) + maximum = item.get("max_args", 0) + if ( + isinstance(minimum, bool) or isinstance(maximum, bool) + or not isinstance(minimum, int) or not isinstance(maximum, int) + or minimum < 0 or maximum < minimum + ): + minimum, maximum = 0, 0 + functions.append({ + "name": name, + "handler": item.get("handler", ""), + "min_args": minimum, + "max_args": maximum, + "description": item.get("description", ""), + "usage": format_filter_usage(name, minimum, maximum), + }) + rows.append({ + "id": function_id, + "name": metadata.get("name", function_id), + "version": metadata.get("version", "0.0.0"), + "source": metadata.get("source", ""), + "description": metadata.get("description", ""), + "functions": functions, + }) + if json_output: + print(json.dumps(rows, ensure_ascii=False)) + return + cli.console.print("\n[bold cyan]Installed Workflow Functions:[/bold cyan]\n") + if not rows: + cli.console.print("[yellow]No workflow functions installed.[/yellow]") + return + for row in rows: + cli.console.print( + f" • [bold]{cli._escape_markup(str(row['name']))}[/bold] " + f"({cli._escape_markup(str(row['id']))}) v{cli._escape_markup(str(row['version']))}" + ) + for function in row["functions"]: + cli.console.print( + f" - {cli._escape_markup(str(function['usage']))}: " + f"{cli._escape_markup(str(function['description']))}" + ) diff --git a/src/specify_cli/workflows/function/command_remove.py b/src/specify_cli/workflows/function/command_remove.py new file mode 100644 index 0000000000..b1c80a195e --- /dev/null +++ b/src/specify_cli/workflows/function/command_remove.py @@ -0,0 +1,38 @@ +"""CLI for removing installed workflow function packages.""" + +from __future__ import annotations + +from .. import _commands as cli +from .. import _package_install as package_install +from . import function_app +from ._domain import FunctionRegistry, WorkflowFunctionError, _validate_function_id +from .installer import _function_install_transaction, _function_spec + + +@function_app.command("remove") +def workflow_function_remove( + function_id: str = cli.typer.Argument(..., help="Function package ID to remove") +): + """Remove a project workflow function package and its metadata.""" + project_root = cli._require_specify_project() + try: + _validate_function_id(function_id) + outcome = package_install.remove_package( + package_install.RemoveRequest(project_root, function_id), + _function_spec(), + package_install.RemoveHooks( + open_registry=FunctionRegistry, + transaction=lambda root: _function_install_transaction(root), + ), + ) + except WorkflowFunctionError as exc: + cli._print_package_error(exc) + raise cli.typer.Exit(1) from exc + if outcome.orphaned: + cli.console.print( + "[yellow]Warning:[/yellow] " + f"{cli._escape_markup(package_install.orphan_warning(function_id))}" + ) + cli.console.print( + f"[green]✓[/green] Workflow function package '{cli._escape_markup(function_id)}' uninstalled" + ) diff --git a/tests/specify_cli/workflows/function/test_command_add.py b/tests/specify_cli/workflows/function/test_command_add.py new file mode 100644 index 0000000000..af7091d25e --- /dev/null +++ b/tests/specify_cli/workflows/function/test_command_add.py @@ -0,0 +1,290 @@ +"""``specify workflow function add``.""" + +from __future__ import annotations + +import os +import zipfile + +import pytest +from typer.testing import CliRunner + +from specify_cli import app +from specify_cli.workflows.function import FunctionRegistry + +from .helpers import ( + TRIM_CODE, + install_dir, + serve_files, + write_installed, + write_package, + zip_package, +) + + +@pytest.fixture +def cli(project_dir, monkeypatch): + monkeypatch.chdir(project_dir) + runner = CliRunner() + return lambda *args, **kw: runner.invoke(app, ["workflow", "function", *args], **kw) + + +@pytest.fixture +def source(tmp_path): + return write_package(tmp_path / "src", "text") + + +class TestDevSource: + def test_installs_from_a_local_directory(self, cli, project_dir, source): + result = cli("add", "text", "--dev", str(source)) + assert result.exit_code == 0, result.output + assert "Workflow function package 'text' installed" in result.output + assert (install_dir(project_dir, "text") / "__init__.py").read_text() == TRIM_CODE + assert FunctionRegistry(project_dir).get("text")["source"] == "local" + + def test_does_not_import_the_package(self, cli, tmp_path, source): + marker = tmp_path / "imported" + write_package(source, "text", code=f"open({str(marker)!r}, 'w').close()\ndef trim(v):\n return v\n") + assert cli("add", "text", "--dev", str(source)).exit_code == 0 + assert not marker.exists() + + def test_exactly_one_source_is_required(self, cli, project_dir, source): + none = cli("add", "text") + both = cli("add", "text", "--dev", str(source), "--from", "https://example.com/p.zip") + for result in (none, both): + assert result.exit_code == 1 + assert "exactly one of --dev or --from is required" in result.output + assert not install_dir(project_dir, "text").exists() + + def test_missing_id_argument(self, cli, source): + assert cli("add", "--dev", str(source)).exit_code == 2 + + @pytest.mark.parametrize("kind", ["missing", "file"]) + def test_dev_source_must_be_a_directory(self, cli, tmp_path, kind): + path = tmp_path / "nothing" + if kind == "file": + path.write_text("x") + result = cli("add", "text", "--dev", str(path)) + assert result.exit_code == 1 and "--dev source must be a directory" in result.output + + @pytest.mark.skipif(not hasattr(os, "symlink"), reason="symlinks are unavailable") + def test_symlinked_dev_source_is_refused(self, cli, tmp_path, source): + link = tmp_path / "link" + link.symlink_to(source, target_is_directory=True) + result = cli("add", "text", "--dev", str(link)) + assert result.exit_code == 1 and "--dev source must be a directory" in result.output + + def test_existing_package_needs_force(self, cli, project_dir, source): + assert cli("add", "text", "--dev", str(source)).exit_code == 0 + again = cli("add", "text", "--dev", str(source)) + assert again.exit_code == 1 and "already installed" in again.output + forced = cli("add", "text", "--dev", str(source), "--force") + assert forced.exit_code == 0, forced.output + + def test_collision_with_builtin_is_reported(self, cli, project_dir, source): + write_package(source, "text", [{"name": "upper", "handler": "trim"}]) + result = cli("add", "text", "--dev", str(source)) + assert result.exit_code == 1 and "built-in filter" in result.output + assert not install_dir(project_dir, "text").exists() + + def test_invalid_id_is_reported(self, cli, source): + result = cli("add", ".cache", "--dev", str(source)) + assert result.exit_code == 1 and "Invalid function id" in result.output + + def test_error_text_is_rendered_literally(self, cli, source): + (source / "function.yml").write_text("id: text\nfunctions: [[red]x]\n") + result = cli("add", "text", "--dev", str(source)) + assert result.exit_code == 1 and "Error:" in result.output + + def test_requires_a_specify_project(self, tmp_path, monkeypatch, source): + bare = tmp_path / "bare" + bare.mkdir() + monkeypatch.chdir(bare) + result = CliRunner().invoke( + app, ["workflow", "function", "add", "text", "--dev", str(source)] + ) + assert result.exit_code != 0 + assert not (bare / ".specify").exists() + + +class TestUrlSource: + def test_confirmed_install_from_archive_root(self, cli, project_dir, source): + with serve_files({"text.zip": zip_package(source)}) as url: + result = cli("add", "text", "--from", url("text.zip"), input="y\n") + assert result.exit_code == 0, result.output + assert "Untrusted Source" in result.output + assert (install_dir(project_dir, "text") / "function.yml").is_file() + assert FunctionRegistry(project_dir).get("text")["source"] == "url" + + def test_archive_with_one_top_level_directory(self, cli, project_dir, source): + with serve_files({"text.zip": zip_package(source, top_level="text-1.0")}) as url: + result = cli("add", "text", "--from", url("text.zip"), input="y\n") + assert result.exit_code == 0, result.output + assert (install_dir(project_dir, "text") / "__init__.py").is_file() + + def test_declined_prompt_installs_nothing_and_makes_no_request(self, cli, project_dir, source): + with serve_files({"text.zip": zip_package(source)}) as url: + result = cli("add", "text", "--from", url("text.zip"), input="n\n") + requests = list(url.requests) + assert result.exit_code == 0 and "Cancelled" in result.output + assert not install_dir(project_dir, "text").exists() + assert requests == [] + + def test_no_request_is_made_before_confirmation(self, cli, source, monkeypatch): + calls: list[str] = [] + from specify_cli.authentication import http as auth_http + + def spy(url, *args, **kwargs): + calls.append(url) + raise AssertionError("network used") + + monkeypatch.setattr(auth_http, "open_url", spy) + result = cli("add", "text", "--from", "https://example.com/text.zip", input="n\n") + assert result.exit_code == 0 and calls == [] + + @pytest.mark.parametrize( + "url", ["http://example.com/p.zip", "ftp://example.com/p.zip", "file:///tmp/p.zip", "https:///p.zip", "not a url"] + ) + def test_insecure_or_invalid_urls_fail_before_the_prompt(self, cli, project_dir, url): + result = cli("add", "text", "--from", url) + assert result.exit_code == 1 + assert "Continue with installation?" not in result.output + assert not install_dir(project_dir, "text").exists() + + def test_already_installed_fails_before_the_prompt(self, cli, source): + assert cli("add", "text", "--dev", str(source)).exit_code == 0 + with serve_files({"text.zip": zip_package(source)}) as url: + result = cli("add", "text", "--from", url("text.zip")) + assert result.exit_code == 1 + assert "already installed" in result.output + assert "Continue with installation?" not in result.output + + def test_force_replaces_from_url(self, cli, project_dir, source, tmp_path): + assert cli("add", "text", "--dev", str(source)).exit_code == 0 + new = write_package(tmp_path / "new", "text", version="2.0.0") + with serve_files({"text.zip": zip_package(new)}) as url: + result = cli("add", "text", "--from", url("text.zip"), "--force", input="y\n") + assert result.exit_code == 0, result.output + assert FunctionRegistry(project_dir).get("text")["version"] == "2.0.0" + + def test_http_error(self, cli, project_dir): + with serve_files({}) as url: + result = cli("add", "text", "--from", url("missing.zip"), input="y\n") + assert result.exit_code == 1 and "Failed to install function from URL: HTTP Error 404" in result.output + assert not install_dir(project_dir, "text").exists() + + @pytest.mark.parametrize("payload", [b"not an archive", b""]) + def test_invalid_archive(self, cli, project_dir, payload): + with serve_files({"text.zip": payload}) as url: + result = cli("add", "text", "--from", url("text.zip"), input="y\n") + assert result.exit_code == 1 and "Failed to install function from URL" in result.output + assert not install_dir(project_dir, "text").exists() + + def test_archive_without_a_manifest(self, cli, project_dir, tmp_path): + (tmp_path / "empty").mkdir() + (tmp_path / "empty" / "README").write_text("x") + with serve_files({"text.zip": zip_package(tmp_path / "empty")}) as url: + result = cli("add", "text", "--from", url("text.zip"), input="y\n") + assert result.exit_code == 1 and "function.yml at its root" in result.output + + def test_path_traversal_archive_is_rejected(self, cli, project_dir, tmp_path, source): + import io + + buffer = io.BytesIO() + with zipfile.ZipFile(buffer, "w") as archive: + archive.writestr("../evil.py", "x") + archive.write(source / "function.yml", "function.yml") + archive.write(source / "__init__.py", "__init__.py") + with serve_files({"text.zip": buffer.getvalue()}) as url: + result = cli("add", "text", "--from", url("text.zip"), input="y\n") + assert result.exit_code == 1 + assert not (project_dir / ".specify" / "workflows" / "evil.py").exists() + assert not (project_dir.parent / "evil.py").exists() + assert not install_dir(project_dir, "text").exists() + + def test_archive_collision_is_reported(self, cli, project_dir, tmp_path): + write_installed(project_dir, "alpha") + FunctionRegistry(project_dir).add("alpha", {"functions": [{"name": "trim"}]}) + with serve_files({"b.zip": zip_package(write_package(tmp_path / "b", "beta"))}) as url: + result = cli("add", "beta", "--from", url("b.zip"), input="y\n") + assert result.exit_code == 1 and "already provided by package 'alpha'" in result.output + assert not install_dir(project_dir, "beta").exists() + + + def test_archive_format_mismatch_is_rejected(self, cli, project_dir, source): + with serve_files({"text.tar.gz": zip_package(source)}) as url: + result = cli("add", "text", "--from", url("text.tar.gz"), input="y\n") + assert result.exit_code == 1 + assert not install_dir(project_dir, "text").exists() + + +class TestSharedUrlFlow: + """``--from`` uses the same trust and download flow as step packages.""" + + def test_trust_prompt_names_the_kind_and_shows_the_url(self, cli, source): + with serve_files({"text.zip": zip_package(source)}) as url: + target = url("text.zip") + result = cli("add", "text", "--from", target, input="n\n") + output = " ".join(result.output.split()) + assert "Untrusted Source" in output and "function package" in output + assert target in result.output + assert "Only install function packages from sources you trust." in output + + def test_both_commands_delegate_to_the_shared_flow(self, project_dir, monkeypatch, source): + from specify_cli.workflows import _archive_install as archive + from specify_cli.workflows.function import command_add as function_add + from specify_cli.workflows.step import command_add as step_add + + calls = [] + + def spy(request, spec, hooks): + calls.append(spec.noun) + return {"name": request.package_id} + + monkeypatch.setattr(archive, "install_from_archive_url", spy) + function_add._install_from_url(project_dir, "text", "https://example.com/t.zip", False) + step_add._install_from_url(project_dir, "my-step", "https://example.com/s.zip", force=False) + assert calls == ["Function", "Step"] + + def test_content_type_and_name_must_agree(self, cli, project_dir, source): + with serve_files({"text.tar.gz": zip_package(source)}) as url: + result = cli("add", "text", "--from", url("text.tar.gz"), input="y\n") + assert result.exit_code == 1 and "Archive format mismatch" in result.output + assert not install_dir(project_dir, "text").exists() + + def test_temporary_archive_directories_are_removed(self, cli, source, tmp_path, monkeypatch): + import tempfile + + created = [] + real = tempfile.TemporaryDirectory + + class Tracking(real): + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + created.append(self.name) + + monkeypatch.setattr(tempfile, "TemporaryDirectory", Tracking) + with serve_files({"text.zip": zip_package(source)}) as url: + assert cli("add", "text", "--from", url("text.zip"), input="y\n").exit_code == 0 + import os + + assert any("speckit-function-archive-" in n for n in created) + assert not [n for n in created if os.path.exists(n)] + + def test_install_error_notes_are_rendered_as_warnings(self): + from specify_cli.workflows import _commands as commands + + error = ValueError("boom [red]") + error.add_note("residue at /tmp/x") + from io import StringIO + + from rich.console import Console + + buffer = StringIO() + original = commands.console + commands.console = Console(file=buffer, width=200, force_terminal=False) + try: + commands._print_package_error(error) + finally: + commands.console = original + text = buffer.getvalue() + assert "Warning: residue at /tmp/x" in text and "Error: boom [red]" in text diff --git a/tests/specify_cli/workflows/function/test_command_list.py b/tests/specify_cli/workflows/function/test_command_list.py new file mode 100644 index 0000000000..dbdbad80ce --- /dev/null +++ b/tests/specify_cli/workflows/function/test_command_list.py @@ -0,0 +1,138 @@ +"""``specify workflow function list``.""" + +from __future__ import annotations + +import json + +import pytest +from typer.testing import CliRunner + +from specify_cli import app +from specify_cli.workflows.function import FunctionRegistry + +from .helpers import functions_dir, write_package + + +@pytest.fixture +def cli(project_dir, monkeypatch): + monkeypatch.chdir(project_dir) + runner = CliRunner() + return lambda *args: runner.invoke(app, ["workflow", "function", *args]) + + +def install(cli, tmp_path, function_id, functions, **manifest): + source = write_package(tmp_path / function_id, function_id, functions, **manifest) + result = cli("add", function_id, "--dev", str(source)) + assert result.exit_code == 0, result.output + + +class TestList: + def test_empty_text_and_json(self, cli): + text = cli("list") + assert text.exit_code == 0 and "No workflow functions installed." in text.output + data = cli("list", "--json") + assert data.exit_code == 0 and json.loads(data.output) == [] + + def test_json_describes_every_package_and_function(self, cli, tmp_path): + install( + cli, tmp_path, "text", + [ + {"name": "trim", "handler": "trim", "description": "Strip"}, + {"name": "pad", "handler": "trim", "min_args": 2, "max_args": 3}, + ], + name="Text", version="1.0.0", description="Helpers", + ) + rows = json.loads(cli("list", "--json").output) + assert rows == [{ + "id": "text", "name": "Text", "version": "1.0.0", "source": "local", + "description": "Helpers", + "functions": [ + {"name": "trim", "handler": "trim", "min_args": 0, "max_args": 0, + "description": "Strip", "usage": "trim"}, + {"name": "pad", "handler": "trim", "min_args": 2, "max_args": 3, + "description": "", "usage": "pad(arg1, arg2, arg3)"}, + ], + }] + + @pytest.mark.parametrize( + ("minimum", "maximum", "usage"), + [ + (0, 0, "f"), + (0, 1, "f or f('x')"), + (1, 1, "f('arg1')"), + (0, 2, "f or f(arg1, arg2)"), + (2, 2, "f(arg1, arg2)"), + ], + ) + def test_usage_for_each_arity(self, cli, tmp_path, minimum, maximum, usage): + install(cli, tmp_path, "p", [{"name": "f", "handler": "trim", "min_args": minimum, "max_args": maximum}]) + (row,) = json.loads(cli("list", "--json").output) + assert row["functions"][0]["usage"] == usage + + def test_packages_are_sorted_by_id(self, cli, tmp_path): + install(cli, tmp_path, "zeta", [{"name": "z", "handler": "trim"}]) + install(cli, tmp_path, "alpha", [{"name": "a", "handler": "trim"}]) + assert [r["id"] for r in json.loads(cli("list", "--json").output)] == ["alpha", "zeta"] + + def test_text_output(self, cli, tmp_path): + install( + cli, tmp_path, "text", + [{"name": "pad", "handler": "trim", "min_args": 1, "max_args": 1, "description": "Pad it"}], + name="Text helpers", version="1.2.3", + ) + result = cli("list") + assert result.exit_code == 0 + assert "Text helpers" in result.output and "(text)" in result.output and "v1.2.3" in result.output + assert "pad('arg1')" in result.output and "Pad it" in result.output + + def test_json_output_is_only_json(self, cli, tmp_path): + install(cli, tmp_path, "text", None) + json.loads(cli("list", "--json").output) + + def test_text_is_rendered_literally(self, cli, tmp_path): + install( + cli, tmp_path, "text", + [{"name": "f", "handler": "trim", "description": "[red]bold[/red] [x]"}], + ) + assert "[red]bold[/red] [x]" in cli("list").output + + def test_listing_never_imports_package_code(self, cli, tmp_path): + marker = tmp_path / "imported" + source = write_package( + tmp_path / "text", "text", + code=f"open({str(marker)!r}, 'w').close()\ndef trim(v):\n return v\n", + ) + assert cli("add", "text", "--dev", str(source)).exit_code == 0 + assert cli("list").exit_code == 0 and cli("list", "--json").exit_code == 0 + assert not marker.exists() + + def test_listing_does_not_register_filters(self, cli, tmp_path): + from specify_cli.workflows.expressions import BUILTIN_FILTERS, FILTER_TABLE + + install(cli, tmp_path, "text", None) + cli("list") + assert set(FILTER_TABLE) == set(BUILTIN_FILTERS) + + def test_damaged_registry_entries_are_tolerated(self, cli, project_dir): + registry = FunctionRegistry(project_dir) + registry.add("odd", { + "functions": [ + "not-a-mapping", + {"name": "bad", "min_args": 5, "max_args": 1}, + {"name": "bool", "min_args": True, "max_args": True}, + ], + }) + registry.data["functions"]["junk"] = "not-a-mapping" + registry.save() + result = cli("list", "--json") + assert result.exit_code == 0 + (row,) = json.loads(result.output) + assert row["id"] == "odd" + assert [(f["name"], f["min_args"], f["max_args"]) for f in row["functions"]] == [ + ("bad", 0, 0), ("bool", 0, 0), + ] + + def test_corrupt_registry_lists_nothing(self, cli, project_dir): + functions_dir(project_dir).mkdir(parents=True) + (functions_dir(project_dir) / "function-registry.json").write_text("{bad") + assert json.loads(cli("list", "--json").output) == [] diff --git a/tests/specify_cli/workflows/function/test_command_remove.py b/tests/specify_cli/workflows/function/test_command_remove.py new file mode 100644 index 0000000000..a1fafa4c59 --- /dev/null +++ b/tests/specify_cli/workflows/function/test_command_remove.py @@ -0,0 +1,174 @@ +"""``specify workflow function remove``.""" + +from __future__ import annotations + +import os +import shutil + +import pytest +from typer.testing import CliRunner + +from specify_cli import app +from specify_cli.workflows.function import FunctionRegistry + +from .helpers import functions_dir, install_dir, write_installed, write_package + + +@pytest.fixture +def cli(project_dir, monkeypatch): + monkeypatch.chdir(project_dir) + runner = CliRunner() + return lambda *args: runner.invoke(app, ["workflow", "function", *args]) + + +def install(cli, tmp_path, function_id="text", name="trim"): + source = write_package(tmp_path / function_id, function_id, [{"name": name, "handler": "trim"}]) + assert cli("add", function_id, "--dev", str(source)).exit_code == 0 + + +class TestRemove: + def test_removes_directory_and_registry_entry(self, cli, project_dir, tmp_path): + install(cli, tmp_path) + result = cli("remove", "text") + assert result.exit_code == 0, result.output + assert "Workflow function package 'text' uninstalled" in result.output + assert not install_dir(project_dir, "text").exists() + assert not FunctionRegistry(project_dir).is_installed("text") + + def test_other_packages_are_untouched(self, cli, project_dir, tmp_path): + install(cli, tmp_path, "alpha", "one") + install(cli, tmp_path, "beta", "two") + assert cli("remove", "alpha").exit_code == 0 + assert install_dir(project_dir, "beta").is_dir() + assert FunctionRegistry(project_dir).is_installed("beta") + + def test_names_are_free_again_after_removal(self, cli, tmp_path): + install(cli, tmp_path, "alpha", "shared") + assert cli("remove", "alpha").exit_code == 0 + install(cli, tmp_path, "beta", "shared") + + def test_unknown_id(self, cli): + result = cli("remove", "ghost") + assert result.exit_code == 1 and "'ghost' is not installed" in result.output + + @pytest.mark.parametrize("bad", ["../x", ".cache", "function-registry.json", "a/b"]) + def test_invalid_id_is_rejected_and_nothing_is_touched(self, cli, project_dir, tmp_path, bad): + install(cli, tmp_path) + result = cli("remove", bad) + assert result.exit_code == 1 and "function id" in result.output + assert install_dir(project_dir, "text").is_dir() + assert (functions_dir(project_dir) / "function-registry.json").is_file() + + def test_unregistered_directory_is_removed(self, cli, project_dir): + write_installed(project_dir, "text") + assert cli("remove", "text").exit_code == 0 + assert not install_dir(project_dir, "text").exists() + + def test_registry_only_entry_is_removed(self, cli, project_dir, tmp_path): + install(cli, tmp_path) + shutil.rmtree(install_dir(project_dir, "text")) + assert cli("remove", "text").exit_code == 0 + assert not FunctionRegistry(project_dir).is_installed("text") + + def test_filesystem_failure_restores_the_registry_entry(self, cli, project_dir, tmp_path, monkeypatch): + install(cli, tmp_path) + + real = shutil.rmtree + target = install_dir(project_dir, "text") + + def fail(path, *args, **kwargs): + if os.fspath(path) == os.fspath(target): + raise OSError("busy") + return real(path, *args, **kwargs) + + monkeypatch.setattr(shutil, "rmtree", fail) + result = cli("remove", "text") + assert result.exit_code == 1 and "Failed to remove function directory" in result.output + assert "busy" in result.output + assert install_dir(project_dir, "text").is_dir() + assert FunctionRegistry(project_dir).is_installed("text") + + @pytest.mark.skipif(not hasattr(os, "symlink"), reason="symlinks are unavailable") + def test_symlinked_package_is_not_followed(self, cli, project_dir, tmp_path): + outside = write_package(tmp_path / "outside", "text") + functions_dir(project_dir).mkdir(parents=True) + install_dir(project_dir, "text").symlink_to(outside, target_is_directory=True) + result = cli("remove", "text") + assert result.exit_code == 1 and "Invalid function id 'text'" in result.output + assert (outside / "function.yml").is_file() + + @pytest.mark.skipif(not hasattr(os, "symlink"), reason="symlinks are unavailable") + def test_symlinked_functions_directory_is_refused(self, cli, project_dir, tmp_path): + outside = tmp_path / "outside" + write_package(outside / "text", "text") + functions_dir(project_dir).symlink_to(outside, target_is_directory=True) + result = cli("remove", "text") + assert result.exit_code == 1 and "symlinked" in result.output + assert (outside / "text" / "function.yml").is_file() + + +class TestSharedRemoval: + """Removal behaves like step removal: same lock, notes and orphan handling.""" + + def test_both_remove_commands_use_the_shared_removal(self): + import inspect + + from specify_cli.workflows.function import command_remove as function_remove + from specify_cli.workflows.step import command_remove as step_remove + + assert "package_install.remove_package(" in inspect.getsource(function_remove) + assert "package_install.remove_package(" in inspect.getsource(step_remove) + assert "remove_package_locked(" in inspect.getsource(step_remove) + + def test_orphaned_directory_warns_and_is_removed(self, cli, project_dir): + write_installed(project_dir, "text") + result = cli("remove", "text") + assert result.exit_code == 0 + output = " ".join(result.output.split()) + assert "'text' has no registry entry" in output and "uninstalled" in output + assert not install_dir(project_dir, "text").exists() + + def test_registered_package_removal_prints_no_orphan_warning(self, cli, tmp_path): + install(cli, tmp_path) + assert "no registry entry" not in cli("remove", "text").output + + def test_directory_failure_keeps_timestamps_when_restoring(self, cli, project_dir, tmp_path, monkeypatch): + install(cli, tmp_path) + before = FunctionRegistry(project_dir).get("text") + real = shutil.rmtree + target = install_dir(project_dir, "text") + + def fail(path, *args, **kwargs): + if os.fspath(path) == os.fspath(target.resolve()): + raise OSError("busy") + return real(path, *args, **kwargs) + + monkeypatch.setattr(shutil, "rmtree", fail) + assert cli("remove", "text").exit_code == 1 + assert FunctionRegistry(project_dir).get("text") == before + + def test_failed_restore_is_reported_as_a_warning_before_the_error(self, cli, project_dir, tmp_path, monkeypatch): + install(cli, tmp_path) + real = shutil.rmtree + target = install_dir(project_dir, "text") + + def fail(path, *args, **kwargs): + if os.fspath(path) == os.fspath(target.resolve()): + raise OSError("busy") + return real(path, *args, **kwargs) + + monkeypatch.setattr(shutil, "rmtree", fail) + monkeypatch.setattr(FunctionRegistry, "restore", lambda *a: (_ for _ in ()).throw(OSError("no disk"))) + result = cli("remove", "text") + output = " ".join(result.output.split()) + assert result.exit_code == 1 + assert output.index("Warning: Failed to restore registry entry for 'text'") < output.index("Error: Failed to remove") + + def test_lock_failure_names_the_package_once(self, cli, project_dir): + lock = project_dir / ".specify" / ".function-install.lock" + lock.mkdir(parents=True, exist_ok=True) + result = cli("remove", "text") + flat = " ".join(result.output.split()) + assert result.exit_code == 1 + assert "Failed to lock function removal 'text'" in flat + assert flat.count("Failed to acquire") == 0 diff --git a/tests/specify_cli/workflows/function/test_installer.py b/tests/specify_cli/workflows/function/test_installer.py new file mode 100644 index 0000000000..02e707be18 --- /dev/null +++ b/tests/specify_cli/workflows/function/test_installer.py @@ -0,0 +1,592 @@ +"""Installer: validate, stage, publish and register without importing code.""" + +from __future__ import annotations + +import os +import shutil +import sys +import threading + +import pytest + +from specify_cli.workflows.function import FunctionRegistry, WorkflowFunctionError +from specify_cli.workflows.function import installer + +from .helpers import ( + functions_dir, + install_dir, + write_installed, + write_package, +) + + +def install(project_dir, source, function_id="text", **kwargs): + return installer.install_function_package( + project_dir, function_id, source, source=kwargs.pop("kind", "local"), **kwargs + ) + + +@pytest.fixture +def source(tmp_path): + return write_package(tmp_path / "src", "text") + + +def leftovers(project_dir): + return [p.name for p in functions_dir(project_dir).iterdir() if p.name.startswith(".speckit")] + + +class TestInstall: + def test_installs_files_and_registers_metadata(self, project_dir, source): + (source / "helpers.py").write_text("X = 1\n") + (source / "sub").mkdir() + (source / "sub" / "data.txt").write_text("d") + write_package( + source, "text", + [{"name": "pad", "handler": "trim", "min_args": 1, "max_args": 2, + "description": "Pad"}], + name="Text", version="1.0.0", description="Helpers", + ) + entry = install(project_dir, source) + + target = install_dir(project_dir, "text") + assert (target / "function.yml").is_file() and (target / "__init__.py").is_file() + assert (target / "helpers.py").read_text() == "X = 1\n" + assert (target / "sub" / "data.txt").read_text() == "d" + assert entry == { + "name": "Text", "version": "1.0.0", "description": "Helpers", "source": "local", + "functions": [{"name": "pad", "handler": "trim", "min_args": 1, + "max_args": 2, "description": "Pad"}], + } + stored = FunctionRegistry(project_dir).get("text") + assert {k: stored[k] for k in entry} == entry + assert leftovers(project_dir) == [] + + def test_name_defaults_to_id(self, project_dir, source): + assert install(project_dir, source)["name"] == "text" + + def test_source_directory_is_left_untouched(self, project_dir, source): + before = sorted(p.name for p in source.iterdir()) + install(project_dir, source) + assert sorted(p.name for p in source.iterdir()) == before + + def test_never_imports_package_code(self, project_dir, tmp_path): + marker = tmp_path / "imported" + write_package( + tmp_path / "src", "text", + code=f"open({str(marker)!r}, 'w').close()\ndef trim(v):\n return v\n", + ) + install(project_dir, tmp_path / "src") + installer.validate_function_package(tmp_path / "src", "text") + assert not marker.exists() + assert not [n for n in sys.modules if n.startswith("_speckit_workflow_function_")] + + def test_url_source_label(self, project_dir, source): + assert install(project_dir, source, kind="url")["source"] == "url" + + @pytest.mark.parametrize("kind", ["", "catalog", "LOCAL", None]) + def test_unknown_source_kind_is_rejected(self, project_dir, source, kind): + with pytest.raises(WorkflowFunctionError, match="'local' or 'url'"): + install(project_dir, source, kind=kind) + assert not install_dir(project_dir, "text").exists() + + def test_packages_with_disjoint_exports_coexist(self, project_dir, tmp_path): + write_package(tmp_path / "a", "alpha", [{"name": "one", "handler": "trim"}]) + write_package(tmp_path / "b", "beta", [{"name": "two", "handler": "trim"}]) + install(project_dir, tmp_path / "a", "alpha") + install(project_dir, tmp_path / "b", "beta") + assert sorted(FunctionRegistry(project_dir).list()) == ["alpha", "beta"] + + +class TestReplacement: + def test_existing_package_requires_force(self, project_dir, source): + install(project_dir, source) + with pytest.raises(WorkflowFunctionError, match="already installed. Remove it first with: specify workflow function remove text"): + install(project_dir, source) + + def test_unregistered_leftover_directory_requires_force(self, project_dir, source): + write_installed(project_dir, "text") + FunctionRegistry(project_dir).remove("text") + with pytest.raises(WorkflowFunctionError, match="Function directory already exists"): + install(project_dir, source) + + def test_force_replaces_files_and_metadata(self, project_dir, tmp_path, source): + (source / "old.py").write_text("") + install(project_dir, source) + installed_at = FunctionRegistry(project_dir).get("text")["installed_at"] + + new = write_package( + tmp_path / "new", "text", [{"name": "pad", "handler": "trim"}], version="2.0.0" + ) + install(project_dir, new, force=True) + + target = install_dir(project_dir, "text") + assert not (target / "old.py").exists() + entry = FunctionRegistry(project_dir).get("text") + assert entry["version"] == "2.0.0" and [f["name"] for f in entry["functions"]] == ["pad"] + assert entry["installed_at"] == installed_at + assert leftovers(project_dir) == [] + + def test_force_may_keep_the_same_export_names(self, project_dir, source): + install(project_dir, source) + install(project_dir, source, force=True) + + def test_force_on_a_fresh_install_succeeds(self, project_dir, source): + install(project_dir, source, force=True) + assert FunctionRegistry(project_dir).is_installed("text") + + +class TestExportCollisions: + def test_builtin_name(self, project_dir, tmp_path): + write_package(tmp_path / "s", "text", [{"name": "upper", "handler": "trim"}]) + with pytest.raises(WorkflowFunctionError, match="conflicts with a built-in filter"): + install(project_dir, tmp_path / "s") + assert not install_dir(project_dir, "text").exists() + assert not FunctionRegistry(project_dir).is_installed("text") + + def test_other_package_name(self, project_dir, source, tmp_path): + install(project_dir, source) + write_package(tmp_path / "o", "other") + with pytest.raises(WorkflowFunctionError, match="already provided by package 'text'"): + install(project_dir, tmp_path / "o", "other") + assert not install_dir(project_dir, "other").exists() + + def test_duplicate_name_within_one_manifest(self, project_dir, tmp_path): + write_package( + tmp_path / "s", "text", + [{"name": "a", "handler": "trim"}, {"name": "a", "handler": "trim"}], + ) + with pytest.raises(WorkflowFunctionError, match="more than once"): + install(project_dir, tmp_path / "s") + + def test_force_cannot_claim_another_packages_export(self, project_dir, source, tmp_path): + install(project_dir, source) + write_package(tmp_path / "o", "other", [{"name": "mine", "handler": "trim"}]) + install(project_dir, tmp_path / "o", "other") + write_package(tmp_path / "o2", "other") # now claims text's 'trim' + with pytest.raises(WorkflowFunctionError, match="already provided by package 'text'"): + install(project_dir, tmp_path / "o2", "other", force=True) + assert [f["name"] for f in FunctionRegistry(project_dir).get("other")["functions"]] == ["mine"] + + def test_locked_recheck_uses_fresh_registry_state(self, project_dir, source, tmp_path, monkeypatch): + """A competing install that lands after the preflight is still caught.""" + real = installer._function_install_transaction + + class Racing: + def __init__(self, root): + self.inner = real(root) + + def __enter__(self): + FunctionRegistry(project_dir).add( + "rival", {"functions": [{"name": "trim"}]} + ) + return self.inner.__enter__() + + def __exit__(self, *exc): + return self.inner.__exit__(*exc) + + monkeypatch.setattr(installer, "_function_install_transaction", Racing) + with pytest.raises(WorkflowFunctionError, match="already provided by package 'rival'"): + install(project_dir, source) + assert not install_dir(project_dir, "text").exists() + assert not FunctionRegistry(project_dir).is_installed("text") + assert leftovers(project_dir) == [] + + def test_concurrent_installs_claiming_one_name_allow_only_one(self, project_dir, tmp_path): + sources = { + package_id: write_package(tmp_path / package_id, package_id) + for package_id in ("alpha", "beta", "gamma", "delta") + } + barrier = threading.Barrier(len(sources)) + results: dict[str, object] = {} + + def worker(package_id): + barrier.wait() + try: + install(project_dir, sources[package_id], package_id) + results[package_id] = "ok" + except WorkflowFunctionError as exc: + results[package_id] = exc + + threads = [threading.Thread(target=worker, args=(i,)) for i in sources] + for thread in threads: + thread.start() + for thread in threads: + thread.join(timeout=30) + + winners = [i for i, r in results.items() if r == "ok"] + assert len(winners) == 1 and len(results) == len(sources) + assert list(FunctionRegistry(project_dir).list()) == winners + assert [p.name for p in functions_dir(project_dir).iterdir() if p.is_dir()] == winners + assert all("already provided" in str(r) for i, r in results.items() if i not in winners) + + +class TestSharedInstallFlow: + """The staged flow, copy and replacement are the step installer's mechanics.""" + + def test_function_installer_delegates_to_the_shared_module(self): + import inspect + + from specify_cli.workflows import _package_install + + assert "_package_install" in inspect.getsource(installer) + assert installer._pi is _package_install + + def test_function_and_step_specs_share_the_same_derived_names(self): + from specify_cli.workflows.step import installer as step_installer + + function_spec, step_spec = installer._function_spec(), step_installer._step_spec() + assert function_spec.work_prefix == ".speckit-function-install-" + assert step_spec.work_prefix == step_installer._WORK_DIR_PREFIX + assert function_spec.lock_name == ".function-install.lock" + assert (function_spec.directory, step_spec.directory) == ("functions", "steps") + + @pytest.mark.skipif(not hasattr(os, "symlink"), reason="symlinks are unavailable") + def test_symlink_swapped_in_after_validation_is_not_followed( + self, project_dir, tmp_path, monkeypatch + ): + source = write_package(tmp_path / "s", "text") + secret = tmp_path / "secret.txt" + secret.write_text("TOP-SECRET") + manifest = installer.validate_function_package(source, "text") + # Simulate the swap: the source validates clean, then gains a symlink. + calls = [] + + def validate_then_swap(path, function_id): + calls.append(path) + if len(calls) == 1: + (source / "leak.txt").symlink_to(secret) + return manifest + + monkeypatch.setattr(installer, "validate_function_package", validate_then_swap) + with pytest.raises(WorkflowFunctionError, match="contains symlink"): + install(project_dir, source) + assert not install_dir(project_dir, "text").exists() + assert leftovers(project_dir) == [] + + @pytest.mark.parametrize("name", ["__pycache__", ".git", ".DS_Store"]) + def test_excluded_names_are_not_installed_or_counted(self, project_dir, source, monkeypatch, name): + monkeypatch.setattr(installer, "_MAX_FUNCTION_PACKAGE_FILES", 2) # the two required files + if name == ".DS_Store": + (source / name).write_text("x") + else: + (source / name).mkdir() + (source / name / "x").write_text("x") + install(project_dir, source) + assert not (install_dir(project_dir, "text") / name).exists() + + def test_locked_recheck_includes_case_insensitive_ids(self, project_dir, source, monkeypatch): + real = installer._function_install_transaction + + class Racing: + def __init__(self, root): + self.inner = real(root) + + def __enter__(self): + FunctionRegistry(project_dir).add("TEXT", {"functions": [{"name": "other"}]}) + return self.inner.__enter__() + + def __exit__(self, *exc): + return self.inner.__exit__(*exc) + + monkeypatch.setattr(installer, "_function_install_transaction", Racing) + with pytest.raises(WorkflowFunctionError, match="collides case-insensitively"): + install(project_dir, source) + assert not install_dir(project_dir, "text").exists() + + def test_staging_directory_is_removed_after_a_failed_install(self, project_dir, source): + (source / "function.yml").write_text("id: text\nfunctions: []\n") + with pytest.raises(WorkflowFunctionError): + install(project_dir, source) + assert not functions_dir(project_dir).exists() or leftovers(project_dir) == [] + + def test_lock_failure_is_reported_as_a_function_error(self, project_dir, source, monkeypatch): + from specify_cli import shared_infra + + def broken(*args, **kwargs): + raise OSError("no lock") + + monkeypatch.setattr(shared_infra, "_exclusive_project_lock", broken) + with pytest.raises(WorkflowFunctionError, match="Failed to acquire the function lock: no lock"): + install(project_dir, source) + assert not install_dir(project_dir, "text").exists() + + +class TestOnDiskClaims: + """Names claimed by package directories count even without a registry entry.""" + + def hand_copy(self, project_dir, package_id="alpha", name="trim", **kwargs): + return write_installed( + project_dir, package_id, [{"name": name, "handler": "trim"}], **kwargs + ) + + def test_unregistered_package_blocks_a_clashing_install(self, project_dir, source): + self.hand_copy(project_dir) + with pytest.raises(WorkflowFunctionError) as info: + install(project_dir, source) + assert "'trim' is already provided by package 'alpha'" in str(info.value) + assert "found on disk, not in the registry" in str(info.value) + assert not install_dir(project_dir, "text").exists() + assert not FunctionRegistry(project_dir).is_installed("text") + + def test_registry_claim_is_reported_without_the_disk_note(self, project_dir, source, tmp_path): + other = write_package(tmp_path / "o", "alpha") + install(project_dir, other, "alpha") + with pytest.raises(WorkflowFunctionError) as info: + install(project_dir, source) + assert "already provided by package 'alpha'" in str(info.value) + assert "found on disk" not in str(info.value) + + def test_lost_registry_does_not_hide_installed_packages(self, project_dir, source, tmp_path): + install(project_dir, write_package(tmp_path / "o", "alpha"), "alpha") + (functions_dir(project_dir) / "function-registry.json").unlink() + with pytest.raises(WorkflowFunctionError, match="found on disk"): + install(project_dir, source) + + def test_disjoint_unregistered_package_does_not_block(self, project_dir, source): + self.hand_copy(project_dir, name="other") + install(project_dir, source) + + @pytest.mark.parametrize("breakage", ["invalid-manifest", "missing-init", "id-mismatch", "unreadable"]) + def test_packages_the_loader_would_skip_claim_nothing(self, project_dir, source, breakage): + package = self.hand_copy(project_dir) + if breakage == "invalid-manifest": + (package / "function.yml").write_text("id: alpha\nfunctions: nope\n") + elif breakage == "missing-init": + (package / "__init__.py").unlink() + elif breakage == "id-mismatch": + write_package(package, "other", [{"name": "trim", "handler": "trim"}]) + else: + (package / "function.yml").write_bytes(b"\xff\xfe") + install(project_dir, source) + + @pytest.mark.skipif(not hasattr(os, "symlink"), reason="symlinks are unavailable") + def test_symlinked_package_claims_nothing(self, project_dir, source, tmp_path): + real = write_package(tmp_path / "elsewhere", "alpha") + functions_dir(project_dir).mkdir(parents=True) + install_dir(project_dir, "alpha").symlink_to(real, target_is_directory=True) + install(project_dir, source) + + def test_non_package_entries_are_ignored(self, project_dir, source): + base = functions_dir(project_dir) + (base / ".cache").mkdir(parents=True) + (base / "stray.txt").write_text("x") + (base / "empty").mkdir() + install(project_dir, source) + + def test_force_does_not_conflict_with_the_package_being_replaced(self, project_dir, source): + install(project_dir, source) + FunctionRegistry(project_dir).remove("text") # now on disk only + install(project_dir, source, force=True) + + def test_force_still_cannot_claim_a_unregistered_neighbours_name(self, project_dir, source): + install(project_dir, source) + self.hand_copy(project_dir, "alpha", name="shout") + replacement = write_package( + source.parent / "new", "text", [{"name": "shout", "handler": "trim"}] + ) + with pytest.raises(WorkflowFunctionError, match="package 'alpha'"): + install(project_dir, replacement, force=True) + assert [f["name"] for f in FunctionRegistry(project_dir).get("text")["functions"]] == ["trim"] + + def test_locked_recheck_sees_a_package_copied_in_after_the_preflight( + self, project_dir, source, monkeypatch + ): + real = installer._function_install_transaction + hand_copy = self.hand_copy + + class Racing: + def __init__(self, root): + self.inner = real(root) + + def __enter__(self): + hand_copy(project_dir) + return self.inner.__enter__() + + def __exit__(self, *exc): + return self.inner.__exit__(*exc) + + monkeypatch.setattr(installer, "_function_install_transaction", Racing) + with pytest.raises(WorkflowFunctionError, match="found on disk"): + install(project_dir, source) + assert not install_dir(project_dir, "text").exists() + + def test_cli_reports_the_disk_claim(self, project_dir, source, monkeypatch): + from typer.testing import CliRunner + + from specify_cli import app + + self.hand_copy(project_dir) + monkeypatch.chdir(project_dir) + result = CliRunner().invoke(app, ["workflow", "function", "add", "text", "--dev", str(source)]) + assert result.exit_code == 1 + assert "found on disk, not in the registry" in " ".join(result.output.split()) + + +class TestPackageValidation: + @pytest.mark.parametrize("missing", ["function.yml", "__init__.py"]) + def test_required_root_files(self, project_dir, source, missing): + (source / missing).unlink() + with pytest.raises(WorkflowFunctionError, match=f"missing required file '{missing}'"): + install(project_dir, source) + + def test_missing_source_directory(self, project_dir, tmp_path): + with pytest.raises(WorkflowFunctionError, match="not found"): + install(project_dir, tmp_path / "nope") + + def test_manifest_id_must_match(self, project_dir, source): + with pytest.raises(WorkflowFunctionError, match="does not match"): + install(project_dir, source, "other") + + def test_invalid_manifest(self, project_dir, source): + (source / "function.yml").write_text("id: text\nfunctions: []\n") + with pytest.raises(WorkflowFunctionError, match="non-empty list"): + install(project_dir, source) + + @pytest.mark.parametrize("bad", [".cache", "function-registry.json", "../x", "a/b", ".hidden", ""]) + def test_invalid_and_reserved_ids(self, project_dir, source, bad): + with pytest.raises(WorkflowFunctionError, match="function id"): + install(project_dir, source, bad) + + def test_unreadable_manifest(self, project_dir, source): + (source / "function.yml").write_bytes(b"\xff\xfe") + with pytest.raises(WorkflowFunctionError, match="Invalid function.yml"): + install(project_dir, source) + + def test_entry_count_limit(self, project_dir, source, monkeypatch): + monkeypatch.setattr(installer, "_MAX_FUNCTION_PACKAGE_FILES", 3) + for index in range(3): + (source / f"f{index}.py").write_text("") + with pytest.raises(WorkflowFunctionError, match="too many entries"): + install(project_dir, source) + assert not install_dir(project_dir, "text").exists() + + def test_size_limit(self, project_dir, source, monkeypatch): + monkeypatch.setattr(installer, "_MAX_FUNCTION_PACKAGE_BYTES", 100) + (source / "big.bin").write_bytes(b"x" * 200) + with pytest.raises(WorkflowFunctionError, match="total size limit"): + install(project_dir, source) + + def test_depth_limit(self, project_dir, source, monkeypatch): + monkeypatch.setattr(installer, "_MAX_FUNCTION_PACKAGE_DEPTH", 2) + (source / "a" / "b" / "c").mkdir(parents=True) + with pytest.raises(WorkflowFunctionError, match="directory depth limit"): + install(project_dir, source) + + def test_limits_allow_a_package_at_the_boundary(self, project_dir, source, monkeypatch): + monkeypatch.setattr(installer, "_MAX_FUNCTION_PACKAGE_DEPTH", 2) + (source / "a" / "b").mkdir(parents=True) + install(project_dir, source) + + +@pytest.mark.skipif(not hasattr(os, "symlink"), reason="symlinks are unavailable") +class TestSymlinks: + def test_symlinked_source_directory(self, project_dir, source, tmp_path): + link = tmp_path / "link" + link.symlink_to(source, target_is_directory=True) + with pytest.raises(WorkflowFunctionError, match="symlinked package directory"): + install(project_dir, link) + + @pytest.mark.parametrize("name", ["function.yml", "__init__.py"]) + def test_symlinked_required_file(self, project_dir, source, tmp_path, name): + real = tmp_path / "real" + shutil.move(source / name, real) + (source / name).symlink_to(real) + with pytest.raises(WorkflowFunctionError, match="must be a regular file, not a symlink"): + install(project_dir, source) + + def test_symlinked_nested_file_or_directory(self, project_dir, source, tmp_path): + outside = tmp_path / "outside.txt" + outside.write_text("secret") + (source / "link.txt").symlink_to(outside) + with pytest.raises(WorkflowFunctionError, match="contains symlink"): + install(project_dir, source) + (source / "link.txt").unlink() + (source / "dir").symlink_to(tmp_path, target_is_directory=True) + with pytest.raises(WorkflowFunctionError, match="contains symlink"): + install(project_dir, source) + assert not install_dir(project_dir, "text").exists() + + @pytest.mark.parametrize("link", [".specify/workflows/functions"]) + def test_symlinked_functions_directory(self, project_dir, source, tmp_path, link): + outside = tmp_path / "outside" + outside.mkdir() + (project_dir / link).symlink_to(outside, target_is_directory=True) + with pytest.raises(WorkflowFunctionError, match="symlinked function directory"): + install(project_dir, source) + assert list(outside.iterdir()) == [] + + def test_symlinked_destination(self, project_dir, source, tmp_path): + functions_dir(project_dir).mkdir() + outside = tmp_path / "outside" + outside.mkdir() + install_dir(project_dir, "text").symlink_to(outside, target_is_directory=True) + with pytest.raises(WorkflowFunctionError, match="through a symlinked path"): + install(project_dir, source, force=True) + assert list(outside.iterdir()) == [] + + def test_functions_path_that_is_a_file(self, project_dir, source): + functions_dir(project_dir).write_text("x") + with pytest.raises(WorkflowFunctionError, match="not a directory"): + install(project_dir, source) + + +class TestDestinationSafety: + def test_source_inside_the_destination_is_rejected(self, project_dir): + inside = write_installed(project_dir, "text") + with pytest.raises(WorkflowFunctionError, match="resolves to the install destination"): + install(project_dir, inside, force=True) + assert (inside / "function.yml").is_file() + + def test_source_containing_the_destination_is_rejected(self, tmp_path): + project = tmp_path / "src" + write_package(project, "text") + (project / ".specify" / "workflows").mkdir(parents=True) + with pytest.raises(WorkflowFunctionError, match="resolves to the install destination"): + install(project, project) + + def test_case_insensitive_id_collisions(self, project_dir, source, tmp_path): + install(project_dir, source) + write_package(tmp_path / "t", "Text", [{"name": "other", "handler": "trim"}]) + with pytest.raises(WorkflowFunctionError, match="collides case-insensitively"): + install(project_dir, tmp_path / "t", "Text") + + def test_registry_failure_removes_the_published_package(self, project_dir, source, monkeypatch): + def fail(self, function_id, metadata): + raise OSError("disk full") + + monkeypatch.setattr(FunctionRegistry, "add", fail) + with pytest.raises(WorkflowFunctionError, match="disk full"): + install(project_dir, source) + assert not install_dir(project_dir, "text").exists() + assert leftovers(project_dir) == [] + + +class TestResolvePackageRoot: + def test_root_with_manifest(self, tmp_path): + write_package(tmp_path, "text") + assert installer.resolve_package_root(tmp_path) == tmp_path + + def test_single_top_level_directory(self, tmp_path): + write_package(tmp_path / "pkg", "text") + assert installer.resolve_package_root(tmp_path) == tmp_path / "pkg" + + @pytest.mark.parametrize("layout", ["empty", "two-dirs", "nested-no-manifest", "file-only"]) + def test_ambiguous_or_missing_manifest(self, tmp_path, layout): + if layout == "two-dirs": + write_package(tmp_path / "a", "a") + write_package(tmp_path / "b", "b") + elif layout == "nested-no-manifest": + (tmp_path / "pkg").mkdir() + elif layout == "file-only": + (tmp_path / "x.txt").write_text("") + with pytest.raises(WorkflowFunctionError, match="function.yml at its root"): + installer.resolve_package_root(tmp_path) + + @pytest.mark.skipif(not hasattr(os, "symlink"), reason="symlinks are unavailable") + def test_symlinked_manifest_or_directory_is_not_a_package(self, tmp_path): + real = write_package(tmp_path / "real", "text") + root = tmp_path / "root" + root.mkdir() + (root / "pkg").symlink_to(real, target_is_directory=True) + with pytest.raises(WorkflowFunctionError): + installer.resolve_package_root(root) diff --git a/tests/specify_cli/workflows/function/test_wiring.py b/tests/specify_cli/workflows/function/test_wiring.py new file mode 100644 index 0000000000..f9499578a0 --- /dev/null +++ b/tests/specify_cli/workflows/function/test_wiring.py @@ -0,0 +1,269 @@ +"""Run, resume, add and update load project functions before using expressions.""" + +from __future__ import annotations + +import json + +import pytest +import yaml +from typer.testing import CliRunner + +from specify_cli import app +from specify_cli.workflows.expressions import BUILTIN_FILTERS, FILTER_TABLE + +from .helpers import install_dir, write_installed + +COLLIDING = [{"name": "upper", "handler": "trim"}] + + +def custom_filters(): + return sorted(set(FILTER_TABLE) - set(BUILTIN_FILTERS)) + + +def workflow_yaml(steps, workflow_id="fn-wf", inputs=None): + data = { + "schema_version": "1.0", + "workflow": {"id": workflow_id, "name": "Fn", "version": "1.0.0"}, + "steps": steps, + } + if inputs: + data["inputs"] = inputs + return yaml.safe_dump(data) + + +@pytest.fixture +def cli(project_dir, monkeypatch): + monkeypatch.chdir(project_dir) + runner = CliRunner() # stdout and stderr are captured separately + return lambda *args, **kw: runner.invoke(app, ["workflow", *args], **kw) + + +@pytest.fixture +def trim_wf(project_dir): + path = project_dir / "wf.yml" + path.write_text( + workflow_yaml( + [{"id": "w", "type": "shell", "run": "printf '%s' '{{ inputs.s | trim }}' > out.txt"}], + inputs={"s": {"type": "string", "default": " hi "}}, + ), + encoding="utf-8", + ) + return path + + +class TestRun: + def test_functions_are_available_to_the_run(self, cli, project_dir, trim_wf): + write_installed(project_dir, "text") + result = cli("run", str(trim_wf), "--json") + assert result.exit_code == 0, result.output + assert json.loads(result.stdout)["status"] == "completed" + assert (project_dir / "out.txt").read_text() == "hi" + + def test_unknown_filter_fails_the_run_without_the_package(self, cli, project_dir, trim_wf): + result = cli("run", str(trim_wf), "--json") + assert result.exit_code != 0 + assert not (project_dir / "out.txt").exists() + + def test_collision_text_mode(self, cli, project_dir, trim_wf): + write_installed(project_dir, "text", COLLIDING) + result = cli("run", str(trim_wf)) + assert result.exit_code == 1 + assert "collides" in result.output and "'upper'" in result.output + assert "rename or remove the package" in result.output + assert not (project_dir / "out.txt").exists() + + def test_collision_json_mode(self, cli, project_dir, trim_wf): + write_installed(project_dir, "text", COLLIDING) + result = cli("run", str(trim_wf), "--json") + assert result.exit_code == 1 + payload = json.loads(result.stdout) + assert payload["status"] == "failed" and "collides" in payload["error"] + assert not (project_dir / "out.txt").exists() + + def test_import_time_output_does_not_contaminate_json(self, cli, project_dir, trim_wf): + write_installed( + project_dir, "text", + code="print('IMPORT-TIME-NOISE')\ndef trim(v):\n return v.strip()\n", + ) + result = cli("run", str(trim_wf), "--json") + assert result.exit_code == 0, result.output + assert "IMPORT-TIME-NOISE" not in result.stdout + assert json.loads(result.stdout)["status"] == "completed" + + def test_broken_package_does_not_block_the_run(self, cli, project_dir, trim_wf): + write_installed(project_dir, "text") + write_installed(project_dir, "broken", [{"name": "x", "handler": "absent"}]) + assert cli("run", str(trim_wf), "--json").exit_code == 0 + + +class TestResume: + @pytest.fixture + def failing_wf(self, project_dir): + calls = project_dir / "calls.txt" + write_installed( + project_dir, "count", + [{"name": "mark", "handler": "mark"}], + code=( + "def mark(value):\n" + f" with open({str(calls)!r}, 'a') as handle:\n" + " handle.write('x')\n" + " return value\n" + ), + ) + path = project_dir / "wf.yml" + path.write_text( + workflow_yaml( + [ + {"id": "done", "type": "shell", "run": "echo {{ inputs.s | mark }}"}, + {"id": "broken", "type": "shell", "run": "echo {{ inputs.s | mark }}; exit 1"}, + ], + inputs={"s": {"type": "string", "default": "v"}}, + ), + encoding="utf-8", + ) + return path, calls + + def start(self, cli, path): + result = cli("run", str(path), "--json") + assert result.exit_code != 0, result.output + return json.loads(result.stdout)["run_id"] + + def test_completed_steps_are_not_replayed_but_the_unfinished_step_is(self, cli, failing_wf): + path, calls = failing_wf + run_id = self.start(cli, path) + assert calls.read_text() == "xx" # both steps evaluated once + resumed = cli("resume", run_id, "--json") + assert resumed.exit_code != 0 + assert calls.read_text() == "xxx" # only the failed step is re-evaluated + + def test_resume_loads_functions(self, cli, project_dir, failing_wf): + path, _ = failing_wf + run_id = self.start(cli, path) + from specify_cli.workflows.function.loader import _unload_custom_functions + + _unload_custom_functions() + assert custom_filters() == [] + cli("resume", run_id, "--json") + assert custom_filters() == ["mark"] + + @pytest.mark.parametrize("json_mode", [False, True]) + def test_resume_collision(self, cli, project_dir, failing_wf, json_mode): + path, calls = failing_wf + run_id = self.start(cli, path) + write_installed(project_dir, "clash", COLLIDING) + args = ["resume", run_id] + (["--json"] if json_mode else []) + result = cli(*args) + assert result.exit_code == 1 + assert calls.read_text() == "xx" + if json_mode: + payload = json.loads(result.stdout) + assert payload["status"] == "failed" and "collides" in payload["error"] + assert payload["run_id"] == run_id + else: + assert "collides" in result.output + + +class TestAddAndUpdate: + @pytest.fixture + def workflow_dir(self, tmp_path): + directory = tmp_path / "wf-src" + directory.mkdir() + (directory / "workflow.yml").write_text( + workflow_yaml([{"id": "a", "type": "shell", "run": "echo hi"}], workflow_id="added"), + encoding="utf-8", + ) + return directory + + def test_add_loads_functions(self, cli, project_dir, workflow_dir): + write_installed(project_dir, "text") + result = cli("add", str(workflow_dir), "--dev") + assert result.exit_code == 0, result.output + assert custom_filters() == ["trim"] + + def test_add_collision(self, cli, project_dir, workflow_dir): + write_installed(project_dir, "text", COLLIDING) + result = cli("add", str(workflow_dir), "--dev") + assert result.exit_code == 1 and "collides" in result.output + assert not (project_dir / ".specify" / "workflows" / "added").exists() + + def test_update_loads_functions(self, cli, project_dir): + write_installed(project_dir, "text") + cli("update") + assert custom_filters() == ["trim"] + + def test_update_collision(self, cli, project_dir): + write_installed(project_dir, "text", COLLIDING) + result = cli("update") + assert result.exit_code == 1 and "collides" in result.output + + +class TestLoadingBoundaries: + def test_function_commands_do_not_import_packages(self, cli, project_dir): + marker = project_dir / "imported" + write_installed( + project_dir, "text", + code=f"open({str(marker)!r}, 'w').close()\ndef trim(v):\n return v\n", + ) + for args in (("function", "list"), ("function", "list", "--json"), ("list",)): + cli(*args) + assert not marker.exists() + + def test_removing_a_package_removes_its_filter_on_next_load(self, cli, project_dir, trim_wf): + write_installed(project_dir, "text") + assert cli("run", str(trim_wf), "--json").exit_code == 0 + assert cli("function", "remove", "text").exit_code == 0 + assert not install_dir(project_dir, "text").exists() + (project_dir / "out.txt").unlink() + assert cli("run", str(trim_wf), "--json").exit_code != 0 + + +class TestLoadFailures: + """An OSError while loading plugins is rendered, never a raw traceback.""" + + @pytest.fixture(params=["functions", "steps"]) + def failing_loader(self, request, monkeypatch): + def boom(root): + raise OSError("permission denied") + + if request.param == "functions": + from specify_cli.workflows import function + + monkeypatch.setattr(function, "load_workflow_functions", boom) + else: + from specify_cli import workflows + + monkeypatch.setattr(workflows, "load_custom_steps", boom) + return request.param + + @pytest.fixture + def wf(self, project_dir): + path = project_dir / "wf.yml" + path.write_text(workflow_yaml([{"id": "a", "type": "shell", "run": "echo hi"}]), encoding="utf-8") + return path + + @pytest.mark.parametrize("command", ["run", "resume"]) + def test_json_mode_reports_a_failed_payload(self, cli, wf, failing_loader, command): + args = ["run", str(wf), "--json"] if command == "run" else ["resume", "some-run", "--json"] + result = cli(*args) + assert result.exit_code == 1 + assert isinstance(result.exception, SystemExit) + payload = json.loads(result.stdout) + assert payload["status"] == "failed" and "permission denied" in payload["error"] + assert payload["run_id"] == ("some-run" if command == "resume" else None) + + @pytest.mark.parametrize("command", ["run", "resume", "add", "update"]) + def test_text_mode_prints_the_error(self, cli, project_dir, wf, failing_loader, command): + args = { + "run": ["run", str(wf)], + "resume": ["resume", "some-run"], + "add": ["add", str(wf)], + "update": ["update"], + }[command] + result = cli(*args) + assert result.exit_code == 1 + assert isinstance(result.exception, SystemExit) + assert "Error: permission denied" in " ".join(result.output.split()) + assert not (project_dir / ".specify" / "workflows" / "fn-wf").exists() + + def test_a_healthy_load_is_unaffected(self, cli, wf): + assert cli("run", str(wf), "--json").exit_code == 0 From 1df3295e9d931fbe8ae8ab67d230537f60d4b940 Mon Sep 17 00:00:00 2001 From: Markus Date: Sat, 7 Nov 2026 08:39:24 +0100 Subject: [PATCH 05/11] docs(workflows): document workflow functions Add the workflow functions guide, the custom-filter section in the expressions reference, the CLI command table and the design notes for the shared package installation and the function loader. Refs #5285 Assisted-by: pi coding agent (GitHub Copilot, model: claude-sonnet-5.5, autonomous) --- design/workflow-step.md | 60 ++++++++++++++++++ docs/toc.yml | 2 + docs/workflows/cli.md | 14 ++++ docs/workflows/expressions.md | 26 ++++++++ docs/workflows/functions.md | 116 ++++++++++++++++++++++++++++++++++ 5 files changed, 218 insertions(+) create mode 100644 docs/workflows/functions.md diff --git a/design/workflow-step.md b/design/workflow-step.md index 9b5b702665..c56244da3f 100644 --- a/design/workflow-step.md +++ b/design/workflow-step.md @@ -82,6 +82,66 @@ The registry holds one shared instance per type. Concurrent `fan-out` can invoke that instance from multiple threads: keep execution stateless and thread-safe, with per-run data in `config` and `context`, not on `self`. +## Workflow functions + +Workflow functions are a separate project-installed extension point for +expression pipe filters. A package under +`.specify/workflows/functions//` contains `function.yml` and `__init__.py`; +the manifest selects callable attributes explicitly and declares argument +bounds. Function names share a flat namespace with the immutable built-in +filter snapshot and other loaded packages. Built-in and duplicate exports are +installation and load errors; built-ins are never mutated. + +The function loader clears the previous project's custom filter registrations, +validates all manifests and collisions before importing code, then imports each +package under a synthetic package name so relative imports work. A package's +exports are registered all-or-nothing and failed imports are cleaned from +`sys.modules`. Installation validates and stages files without importing them; +run, resume, workflow add, and workflow update load functions before workflow +validation. Loading executable Python is not a sandbox or trust endorsement. + +Function handlers are wrapped at registration to normalize ordinary +`Exception` failures to a `ValueError` prefixed with the export name. This is +deliberately function-specific; `BaseException` is not caught and step +implementations do not gain a new recovery policy. Return values continue +through the existing expression and persistence boundaries without a separate +type whitelist. The process-global filter table is read by concurrent fan-out +work, so handlers must be stateless and thread-safe. + +## Shared package installation + +Step packages and function packages share their install, registry, URL and +removal mechanics instead of each owning a copy: + +- `workflows/_package_install.py` owns path and symlink safety, tree walking and + copying (no-follow, excludes, entry/byte/depth budgets), the project lock, + publish-with-rollback, removal, and one `install_package` flow: reject + conflicts, validate the source before creating directories, stage, re-validate + the staged copy, then re-run every conflict check under the lock immediately + before publishing. It is parameterized by small value objects: + `InstallRequest` (what to install), `PackageSpec` (naming, error type and + limits; directory, lock and staging names derive from the noun), + `PackageRules` (the kind's locate/registry/validate/check/entry semantics) and + `StagingOps` (copy, publish, lock; `StagingOps.standard(spec)` by default). + Removal uses `RemoveRequest` and `RemoveHooks`. +- `workflows/_package_registry.py` provides `PackageRegistry`, the symlink-safe, + atomically written JSON registry with rollback of in-memory state on a failed + write. `StepRegistry` and `FunctionRegistry` only name their file, directory, + record key and error type. +- `workflows/_archive_install.py` is the CLI flow for `--from`: URL validation, + the untrusted-source prompt, GitHub asset resolution, download, archive + format agreement, safe extraction and cleanup. `_commands._print_package_error` + renders failures, including advisory notes as warnings. + +A new package kind supplies a `PackageSpec`, `PackageRules`, a registry subclass +and its manifest parser; it should not copy any of the above. The step +installer keeps its established names (`_copy_package_tree`, `_replace_install`, +`_step_install_transaction`, ...) as thin bindings and builds its spec at call +time from its module limits, so existing patch points and messages are stable. +Function-specific additions stay in `function/`: manifest parsing, export-name +checks against built-ins, the registry and the package directories on disk, and +registry metadata. + ## Adding a step type 1. Implement the step in its own `step//` subpackage and register diff --git a/docs/toc.yml b/docs/toc.yml index 9b5674a6b5..fd0ae9039f 100644 --- a/docs/toc.yml +++ b/docs/toc.yml @@ -137,6 +137,8 @@ href: workflows/api.md - name: Expressions href: workflows/expressions.md + - name: Workflow Functions + href: workflows/functions.md - name: Overlays href: workflows/overlays.md - name: Catalogs diff --git a/docs/workflows/cli.md b/docs/workflows/cli.md index b8a5c148fe..07e1b87458 100644 --- a/docs/workflows/cli.md +++ b/docs/workflows/cli.md @@ -188,6 +188,20 @@ archives follow the same validation and installation behavior. For exact-version catalog selection and its digest requirements, see [Catalog releases](catalogs.md#workflow-releases). +## Workflow Functions + +```bash +specify workflow function add --dev +specify workflow function add --from +specify workflow function list [--json] +specify workflow function remove +``` + +Exactly one source option is required for `add`. Function packages are Python +code and are loaded before workflow validation; install only packages you have +reviewed. `list --json` reports registry metadata and generic export usage +without importing package code. + ## Update Workflows ```bash diff --git a/docs/workflows/expressions.md b/docs/workflows/expressions.md index d020d0b0a4..635acb2169 100644 --- a/docs/workflows/expressions.md +++ b/docs/workflows/expressions.md @@ -13,6 +13,32 @@ Steps can reference inputs and previous step outputs using `{{ expression }}` sy Available filters: `default`, `join`, `contains`, `map`, `from_json`, `to_json`, `upper`, `lower`, `split`, `length`. +## Custom filters (Workflow Functions) + +Projects can install Python callables as pipe filters. See +[Workflow functions](functions.md) for package and CLI details. + +```yaml +# After installing a package that exports trim: +message: "{{ inputs.message | trim | upper }}" +``` + +A function receives the piped value followed by the explicit filter +arguments. Manifest `min_args` and `max_args` bounds are checked during +validation and evaluation. Bare form is available for zero-minimum functions; +parenthesized calls contain one or more arguments. Function names share a flat +namespace with built-in filters and other installed packages. Collisions are +errors and built-ins are never shadowed. + +Function packages contain executable Python and are imported before workflow +validation. Review package source before installing it. Handlers should be +stateless and thread-safe because fan-out can evaluate filters concurrently. +Function failures use the normal expression-error path, with the function name +prefixed to the exception; return values use the existing expression consumer +rules rather than a separate function-specific type contract. As with other +workflow side effects, an interruption before a resume checkpoint can repeat a +handler, so resume is at-least-once. + ## Syntax | Form | Example | diff --git a/docs/workflows/functions.md b/docs/workflows/functions.md new file mode 100644 index 0000000000..08998ce517 --- /dev/null +++ b/docs/workflows/functions.md @@ -0,0 +1,116 @@ +# Workflow functions + +Workflow functions are project-installed Python callables exposed as pipe +filters in workflow expressions. They are installed under +`.specify/workflows/functions//` and consist of a `function.yml` manifest +and an `__init__.py` module. + +## Install and manage functions + +```bash +specify workflow function add text --dev ./workflow-functions/text +specify workflow function list +specify workflow function list --json +specify workflow function remove text +``` + +A package can also be installed from an archive URL: + +```bash +specify workflow function add text --from https://example.test/text.zip +``` + +Exactly one of `--dev` and `--from` is required. `--force` replaces an existing +package with the same package ID; it cannot replace a function export owned by +another package. + +Installation follows the same rules as custom step packages: files are validated +and copied through a private staging directory, symlinks and unsupported file +types are rejected, `.git`, `__pycache__` and `.DS_Store` are not copied, and a +package may contain at most 512 entries, 50 MiB and 32 directory levels. A +`--from` install shows an untrusted-source prompt with the URL before any +request, resolves GitHub release asset URLs, and requires the URL, final URL and +`Content-Type` to agree on the archive format. + +`remove` deletes the package directory and its registry entry under the same +project lock as installation. A directory with no registry entry is removed with +a warning; if the directory cannot be deleted, the registry entry is restored. + +`list` reads the persisted registry only. It does not import package code and +reports each export's generic usage form. A package that was copied into +`.specify/workflows/functions/` by hand is loaded when a workflow runs but is not +listed, because it has no registry entry. + +## Manifest + +```yaml +id: text +name: Text helpers +version: 1.0.0 +description: Small text transformations. +functions: + - name: trim + handler: trim + min_args: 0 + max_args: 0 + description: Remove surrounding whitespace. +``` + +Package IDs use the same safe path-component validation as custom workflow +steps. Export names are lowercase identifiers (`a-z`, `0-9`, and `_`) and must +be unique in a package. `min_args` and `max_args` are non-negative integers; +they count arguments after the value being piped. The defaults are zero. A +bare filter is accepted only when `min_args` is zero. Parenthesized calls must +contain at least one argument and must be within the declared bounds. + +The handler is called as `handler(value, *args)`. For example: + +```python + +def trim(value): + return value.strip() + + +def surround(value, left, right): + return f"{left}{value}{right}" +``` + +```yaml +- id: normalize + type: shell + strict: true + env: + VALUE: "{{ inputs.value | trim }}" + run: 'printf "%s\\n" "$VALUE"' +``` + +## Loading and trust + +Installing validates and copies package files but does not import them. Running, +resuming, adding, or updating a workflow loads installed function modules before +workflow validation. Loading executes Python code, so review packages and +install only from sources you trust. Archive downloads use the same HTTPS and +loopback-HTTP restrictions as other executable workflow packages. + +Function names share a flat namespace with built-in and other installed +filters. Installation rejects a name that is a built-in filter or is already +provided by another package, whether that package is in the registry or only +present on disk (the error says "found on disk, not in the registry" in that +case). Runtime loading also fails on a collision, naming every colliding +function and both sources, if packages were copied manually or a new Spec Kit +release introduces a conflicting built-in. The collision check happens before +any package code is imported. Built-in filters are never replaced. If loading +fails because a directory or file cannot be read, the command reports the error +and exits with code 1 (a failed payload with `--json`). + +The filter registry is process-global while a workflow is loaded. A project +reload replaces the previous project's custom filters. Function handlers must +be stateless and thread-safe because fan-out steps can evaluate expressions +concurrently. As with other workflow side effects, an interruption before a +resume checkpoint can cause an operation to run again; resume provides +at-least-once rather than exactly-once execution. + +Custom return values use the existing expression and step-output handling; no +additional function-specific type or JSON whitelist is applied. Exceptions +raised by a handler are normalized as `ValueError` messages prefixed with the +function name, while `BaseException` subclasses are not intercepted. From 62444c9f993cace2e873306f027bae58c7248185 Mon Sep 17 00:00:00 2001 From: Markus Date: Sat, 7 Nov 2026 09:26:40 +0100 Subject: [PATCH 06/11] fix(workflows): cap custom filter argument counts at 16 A function manifest could declare an arbitrarily large max_args. It passed install validation and was stored, and building the usage text then allocated one string per declared argument, so function list and every workflow load could exhaust memory. Validate the bounds in one valid_arg_bounds() (0 <= min_args <= max_args <= MAX_FILTER_ARGS) shared by the manifest parser, register_custom_filter and function list. Oversized stored values are listed as 0..0 like other damaged bounds. Addresses review feedback on #5303. Assisted-by: pi coding agent (GitHub Copilot, model: claude-sonnet-5.5, autonomous) --- docs/workflows/functions.md | 5 ++-- .../workflows/expressions/filters.py | 25 +++++++++++++------ src/specify_cli/workflows/function/_domain.py | 11 +++----- .../workflows/function/command_list.py | 8 ++---- .../workflows/function/test_command_add.py | 9 +++++++ .../workflows/function/test_command_list.py | 16 ++++++++++++ .../workflows/function/test_custom_filters.py | 20 +++++++++++++++ .../workflows/function/test_domain.py | 20 +++++++++++++++ .../workflows/function/test_loader.py | 7 ++++++ 9 files changed, 99 insertions(+), 22 deletions(-) diff --git a/docs/workflows/functions.md b/docs/workflows/functions.md index 08998ce517..882302e3da 100644 --- a/docs/workflows/functions.md +++ b/docs/workflows/functions.md @@ -58,8 +58,9 @@ functions: Package IDs use the same safe path-component validation as custom workflow steps. Export names are lowercase identifiers (`a-z`, `0-9`, and `_`) and must -be unique in a package. `min_args` and `max_args` are non-negative integers; -they count arguments after the value being piped. The defaults are zero. A +be unique in a package. `min_args` and `max_args` are integers with +`0 <= min_args <= max_args <= 16`; they count arguments after the value being +piped. The defaults are zero. A bare filter is accepted only when `min_args` is zero. Parenthesized calls must contain at least one argument and must be within the declared bounds. diff --git a/src/specify_cli/workflows/expressions/filters.py b/src/specify_cli/workflows/expressions/filters.py index 141f0197f0..7dcee8d825 100644 --- a/src/specify_cli/workflows/expressions/filters.py +++ b/src/specify_cli/workflows/expressions/filters.py @@ -287,6 +287,22 @@ class FilterSpec: usage: str = "" +# Largest number of explicit arguments a custom filter may accept. Manifests and +# registry records are untrusted, and usage text is built per argument. +MAX_FILTER_ARGS = 16 + +ARG_BOUNDS_RULE = f"integers with 0 <= min_args <= max_args <= {MAX_FILTER_ARGS}" + + +def valid_arg_bounds(min_args: Any, max_args: Any) -> bool: + """Whether *min_args*/*max_args* are acceptable custom-filter argument bounds.""" + return ( + not isinstance(min_args, bool) and not isinstance(max_args, bool) + and isinstance(min_args, int) and isinstance(max_args, int) + and 0 <= min_args <= max_args <= MAX_FILTER_ARGS + ) + + def format_filter_usage(name: str, min_args: int, max_args: int) -> str: """Return generic usage text for a filter's argument bounds.""" if min_args == max_args == 0: @@ -359,14 +375,9 @@ def register_custom_filter( raise KeyError(f"Filter '{name}' is already registered") if not callable(function): raise TypeError(f"Custom filter '{name}' handler must be callable") - if ( - isinstance(min_args, bool) or isinstance(max_args, bool) - or not isinstance(min_args, int) or not isinstance(max_args, int) - or min_args < 0 or max_args < 0 or min_args > max_args - ): + if not valid_arg_bounds(min_args, max_args): raise ValueError( - f"Invalid argument bounds for custom filter '{name}': expected " - "non-negative integers with min_args <= max_args" + f"Invalid argument bounds for custom filter '{name}': expected {ARG_BOUNDS_RULE}" ) def normalized(value: Any, *args: Any) -> Any: diff --git a/src/specify_cli/workflows/function/_domain.py b/src/specify_cli/workflows/function/_domain.py index 2d38322dfc..ad3c9ee09b 100644 --- a/src/specify_cli/workflows/function/_domain.py +++ b/src/specify_cli/workflows/function/_domain.py @@ -10,7 +10,7 @@ import yaml from .._package_registry import PackageRegistry -from ..expressions.filters import format_filter_usage +from ..expressions.filters import ARG_BOUNDS_RULE, format_filter_usage, valid_arg_bounds from ..step.installer import StepInstallError, validate_step_id @@ -110,13 +110,10 @@ def parse_function_manifest(text: str, expected_id: str | None = None) -> Functi ) minimum = item.get("min_args", 0) maximum = item.get("max_args", 0) - if ( - isinstance(minimum, bool) or isinstance(maximum, bool) - or not isinstance(minimum, int) or not isinstance(maximum, int) - or minimum < 0 or maximum < minimum - ): + if not valid_arg_bounds(minimum, maximum): raise WorkflowFunctionError( - f"function.yml function '{name}' has invalid argument bounds" + f"function.yml function '{name}' has invalid argument bounds: " + f"expected {ARG_BOUNDS_RULE}" ) description = item.get("description", "") if not isinstance(description, str): diff --git a/src/specify_cli/workflows/function/command_list.py b/src/specify_cli/workflows/function/command_list.py index fd985bfe81..e57537c65c 100644 --- a/src/specify_cli/workflows/function/command_list.py +++ b/src/specify_cli/workflows/function/command_list.py @@ -7,7 +7,7 @@ from .. import _commands as cli from . import function_app from ._domain import FunctionRegistry -from ..expressions.filters import format_filter_usage +from ..expressions.filters import format_filter_usage, valid_arg_bounds @function_app.command("list") @@ -30,11 +30,7 @@ def workflow_function_list( name = item.get("name", "") minimum = item.get("min_args", 0) maximum = item.get("max_args", 0) - if ( - isinstance(minimum, bool) or isinstance(maximum, bool) - or not isinstance(minimum, int) or not isinstance(maximum, int) - or minimum < 0 or maximum < minimum - ): + if not valid_arg_bounds(minimum, maximum): minimum, maximum = 0, 0 functions.append({ "name": name, diff --git a/tests/specify_cli/workflows/function/test_command_add.py b/tests/specify_cli/workflows/function/test_command_add.py index af7091d25e..aed4ccf0b1 100644 --- a/tests/specify_cli/workflows/function/test_command_add.py +++ b/tests/specify_cli/workflows/function/test_command_add.py @@ -90,6 +90,15 @@ def test_invalid_id_is_reported(self, cli, source): result = cli("add", ".cache", "--dev", str(source)) assert result.exit_code == 1 and "Invalid function id" in result.output + def test_oversized_max_args_is_rejected_and_nothing_is_installed(self, cli, project_dir, source): + (source / "function.yml").write_text( + "id: text\nfunctions:\n - {name: trim, handler: trim, max_args: 100000000}\n" + ) + result = cli("add", "text", "--dev", str(source)) + assert result.exit_code == 1 and "argument bounds" in result.output + assert not FunctionRegistry(project_dir).is_installed("text") + assert not install_dir(project_dir, "text").exists() + def test_error_text_is_rendered_literally(self, cli, source): (source / "function.yml").write_text("id: text\nfunctions: [[red]x]\n") result = cli("add", "text", "--dev", str(source)) diff --git a/tests/specify_cli/workflows/function/test_command_list.py b/tests/specify_cli/workflows/function/test_command_list.py index dbdbad80ce..06b5572ffc 100644 --- a/tests/specify_cli/workflows/function/test_command_list.py +++ b/tests/specify_cli/workflows/function/test_command_list.py @@ -132,6 +132,22 @@ def test_damaged_registry_entries_are_tolerated(self, cli, project_dir): ("bad", 0, 0), ("bool", 0, 0), ] + def test_oversized_stored_bounds_are_listed_without_expanding_usage(self, cli, project_dir): + registry = FunctionRegistry(project_dir) + registry.add("big", { + "functions": [ + {"name": "huge", "min_args": 0, "max_args": 100_000_000}, + {"name": "edge", "min_args": 0, "max_args": 16}, + ], + }) + result = cli("list", "--json") + assert result.exit_code == 0 + (row,) = json.loads(result.output) + assert [(f["name"], f["min_args"], f["max_args"]) for f in row["functions"]] == [ + ("huge", 0, 0), ("edge", 0, 16), + ] + assert cli("list").exit_code == 0 + def test_corrupt_registry_lists_nothing(self, cli, project_dir): functions_dir(project_dir).mkdir(parents=True) (functions_dir(project_dir) / "function-registry.json").write_text("{bad") diff --git a/tests/specify_cli/workflows/function/test_custom_filters.py b/tests/specify_cli/workflows/function/test_custom_filters.py index bf7a1f4f22..6541cd55c4 100644 --- a/tests/specify_cli/workflows/function/test_custom_filters.py +++ b/tests/specify_cli/workflows/function/test_custom_filters.py @@ -163,6 +163,26 @@ def test_invalid_bounds_are_rejected(self, minimum, maximum): reg("x", minimum=minimum, maximum=maximum) assert "x" not in FILTER_TABLE + @pytest.mark.parametrize(("minimum", "maximum"), [(0, 17), (17, 17), (0, 10**9)]) + def test_bounds_above_the_cap_are_rejected_without_building_usage( + self, minimum, maximum, monkeypatch + ): + from specify_cli.workflows.expressions import filters + + def forbidden(*args): + raise AssertionError("usage text must not be built for rejected bounds") + + monkeypatch.setattr(filters, "format_filter_usage", forbidden) + with pytest.raises(ValueError, match="argument bounds.*<= 16"): + reg("x", minimum=minimum, maximum=maximum) + assert "x" not in FILTER_TABLE + + def test_bounds_at_the_cap_are_accepted(self): + reg("x", minimum=0, maximum=16) + spec = FILTER_TABLE["x"] + assert (spec.min_args, spec.max_args) == (0, 16) + assert spec.usage.startswith("x or x(arg1, arg2") + def test_unregister_removes_only_custom_filters(self): reg("a") reg("b") diff --git a/tests/specify_cli/workflows/function/test_domain.py b/tests/specify_cli/workflows/function/test_domain.py index 9a8cd21bbc..35850e8293 100644 --- a/tests/specify_cli/workflows/function/test_domain.py +++ b/tests/specify_cli/workflows/function/test_domain.py @@ -118,6 +118,26 @@ def test_invalid_bounds(self, minimum, maximum): with pytest.raises(WorkflowFunctionError, match="argument bounds"): parse_function_manifest(data, "text") + @pytest.mark.parametrize("maximum", [17, 1000, 100_000_000]) + def test_max_args_above_the_cap_is_rejected(self, maximum): + data = manifest(functions=[{"name": "a", "handler": "a", "max_args": maximum}]) + with pytest.raises(WorkflowFunctionError, match="argument bounds.*<= 16"): + parse_function_manifest(data, "text") + + def test_min_args_above_the_cap_is_rejected(self): + data = manifest(functions=[ + {"name": "a", "handler": "a", "min_args": 17, "max_args": 17} + ]) + with pytest.raises(WorkflowFunctionError, match="argument bounds"): + parse_function_manifest(data, "text") + + def test_max_args_at_the_cap_is_accepted(self): + data = manifest(functions=[ + {"name": "a", "handler": "a", "min_args": 16, "max_args": 16} + ]) + (export,) = parse_function_manifest(data, "text").exports + assert (export.min_args, export.max_args) == (16, 16) + def test_max_defaults_to_zero_so_min_alone_is_invalid(self): data = manifest(functions=[{"name": "a", "handler": "a", "min_args": 1}]) with pytest.raises(WorkflowFunctionError, match="argument bounds"): diff --git a/tests/specify_cli/workflows/function/test_loader.py b/tests/specify_cli/workflows/function/test_loader.py index 3b5ec07cf8..58afb51662 100644 --- a/tests/specify_cli/workflows/function/test_loader.py +++ b/tests/specify_cli/workflows/function/test_loader.py @@ -156,6 +156,13 @@ def test_invalid_manifest(self, project_dir): (package / "function.yml").write_text("id: bad\nfunctions: nope\n") self.assert_only_healthy_loaded(project_dir) + def test_oversized_max_args_is_skipped_like_any_invalid_manifest(self, project_dir): + self.healthy(project_dir) + write_installed( + project_dir, "bad", [{"name": "huge", "handler": "trim", "max_args": 10**9}] + ) + self.assert_only_healthy_loaded(project_dir) + def test_unreadable_manifest(self, project_dir): self.healthy(project_dir) package = write_installed(project_dir, "bad") From 7ab9570ea4a690b80cc844eb56cc524ad1e5daa0 Mon Sep 17 00:00:00 2001 From: Markus Date: Sat, 7 Nov 2026 09:27:21 +0100 Subject: [PATCH 07/11] fix(workflows): report non-string function.yml keys instead of raising YAML mapping keys can be any scalar. Sorting and joining unknown manifest keys assumed strings, so a numeric or mixed-type key raised TypeError, which bypassed the loader's malformed-package handling and the CLI's WorkflowFunctionError rendering. Stringify unknown keys before sorting and reporting them, at the manifest and function-item level. Addresses review feedback on #5303. Assisted-by: pi coding agent (GitHub Copilot, model: claude-sonnet-5.5, autonomous) --- src/specify_cli/workflows/function/_domain.py | 13 +++++++++++-- .../workflows/function/test_command_add.py | 12 ++++++++++++ .../specify_cli/workflows/function/test_domain.py | 14 ++++++++++++++ .../specify_cli/workflows/function/test_loader.py | 8 ++++++++ 4 files changed, 45 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/workflows/function/_domain.py b/src/specify_cli/workflows/function/_domain.py index ad3c9ee09b..3cb93f3e62 100644 --- a/src/specify_cli/workflows/function/_domain.py +++ b/src/specify_cli/workflows/function/_domain.py @@ -61,6 +61,15 @@ def _string(data: dict[str, Any], key: str, default: str = "") -> str: return value +def _unknown_keys(mapping: dict[Any, Any], allowed: set[str]) -> list[str]: + """Sorted text of the keys outside *allowed*. + + YAML mapping keys can be any scalar, so they are stringified before they + are sorted or joined into a message. + """ + return sorted(str(key) for key in mapping.keys() - allowed) + + def parse_function_manifest(text: str, expected_id: str | None = None) -> FunctionManifest: """Parse the strict ``function.yml`` schema without importing code.""" try: @@ -70,7 +79,7 @@ def parse_function_manifest(text: str, expected_id: str | None = None) -> Functi if not isinstance(raw, dict): raise WorkflowFunctionError("function.yml must be a YAML mapping") allowed = {"id", "name", "version", "description", "functions"} - unknown = sorted(set(raw) - allowed) + unknown = _unknown_keys(raw, allowed) if unknown: raise WorkflowFunctionError(f"function.yml contains unknown key(s): {', '.join(unknown)}") function_id = raw.get("id") @@ -90,7 +99,7 @@ def parse_function_manifest(text: str, expected_id: str | None = None) -> Functi if not isinstance(item, dict): raise WorkflowFunctionError(f"function.yml function at index {index} must be a mapping") allowed_export = {"name", "handler", "min_args", "max_args", "description"} - unknown_export = sorted(set(item) - allowed_export) + unknown_export = _unknown_keys(item, allowed_export) if unknown_export: raise WorkflowFunctionError( f"function.yml function at index {index} contains unknown key(s): {', '.join(unknown_export)}" diff --git a/tests/specify_cli/workflows/function/test_command_add.py b/tests/specify_cli/workflows/function/test_command_add.py index aed4ccf0b1..6b54b76cf3 100644 --- a/tests/specify_cli/workflows/function/test_command_add.py +++ b/tests/specify_cli/workflows/function/test_command_add.py @@ -99,6 +99,18 @@ def test_oversized_max_args_is_rejected_and_nothing_is_installed(self, cli, proj assert not FunctionRegistry(project_dir).is_installed("text") assert not install_dir(project_dir, "text").exists() + @pytest.mark.parametrize("extra", ["1: x", "1: x\nb: y"]) + def test_non_string_manifest_key_is_an_error_not_a_traceback( + self, cli, project_dir, source, extra + ): + (source / "function.yml").write_text( + f"id: text\n{extra}\nfunctions: [{{name: trim, handler: trim}}]\n" + ) + result = cli("add", "text", "--dev", str(source)) + assert result.exit_code == 1 and "unknown key(s)" in result.output + assert "Traceback" not in result.output and not isinstance(result.exception, TypeError) + assert not FunctionRegistry(project_dir).is_installed("text") + def test_error_text_is_rendered_literally(self, cli, source): (source / "function.yml").write_text("id: text\nfunctions: [[red]x]\n") result = cli("add", "text", "--dev", str(source)) diff --git a/tests/specify_cli/workflows/function/test_domain.py b/tests/specify_cli/workflows/function/test_domain.py index 35850e8293..07a32556f0 100644 --- a/tests/specify_cli/workflows/function/test_domain.py +++ b/tests/specify_cli/workflows/function/test_domain.py @@ -107,6 +107,20 @@ def test_duplicate_name_in_one_manifest(self): with pytest.raises(WorkflowFunctionError, match="more than once"): parse_function_manifest(data, "text") + @pytest.mark.parametrize( + ("text", "listed"), + [ + ("id: text\n1: x\nfunctions: [{name: a, handler: a}]\n", "1"), + ("id: text\n1: x\nb: y\nfunctions: [{name: a, handler: a}]\n", "1, b"), + ("id: text\n1.5: x\n~: y\nfunctions: [{name: a, handler: a}]\n", "1.5, None"), + ("id: text\nfunctions: [{name: a, handler: a, 2: x}]\n", "2"), + ("id: text\nfunctions: [{name: a, handler: a, 2: x, c: y}]\n", "2, c"), + ], + ) + def test_non_string_unknown_keys_are_reported_not_raised(self, text, listed): + with pytest.raises(WorkflowFunctionError, match=f"unknown key\\(s\\): {listed}$"): + parse_function_manifest(text, "text") + @pytest.mark.parametrize( ("minimum", "maximum"), [(2, 1), (-1, 0), (0, -1), (True, 1), (0, True), ("1", 2), (0, 1.5), (None, 1)], diff --git a/tests/specify_cli/workflows/function/test_loader.py b/tests/specify_cli/workflows/function/test_loader.py index 58afb51662..7507a8f147 100644 --- a/tests/specify_cli/workflows/function/test_loader.py +++ b/tests/specify_cli/workflows/function/test_loader.py @@ -156,6 +156,14 @@ def test_invalid_manifest(self, project_dir): (package / "function.yml").write_text("id: bad\nfunctions: nope\n") self.assert_only_healthy_loaded(project_dir) + def test_non_string_manifest_key_is_skipped_like_any_invalid_manifest(self, project_dir): + self.healthy(project_dir) + package = write_installed(project_dir, "bad") + (package / "function.yml").write_text( + "id: bad\n1: x\nb: y\nfunctions: [{name: bad, handler: trim}]\n" + ) + self.assert_only_healthy_loaded(project_dir) + def test_oversized_max_args_is_skipped_like_any_invalid_manifest(self, project_dir): self.healthy(project_dir) write_installed( From 97e3b05750ccaf47b7a1310521ea4e4de0e543e0 Mon Sep 17 00:00:00 2001 From: Markus Date: Sat, 7 Nov 2026 12:14:43 +0100 Subject: [PATCH 08/11] test(workflows): make function tests portable to Windows and macOS - Resolve the expected path in the remove-failure test; macOS and Windows temp directories are symlinked or 8.3 paths, so the unresolved path never matched what removal passes to rmtree and the injected error never fired. - Skip the registry mode-bit assertion on Windows, like the existing mode-preservation test. - Run the workflow-wiring shell fixtures through the current interpreter instead of POSIX printf, single quotes and ';' chaining, which cmd.exe does not interpret the same way. Addresses review feedback on #5303. Assisted-by: pi coding agent (GitHub Copilot, model: claude-sonnet-5.5, autonomous) --- .../workflows/function/test_command_remove.py | 2 +- .../function/test_registry_parity.py | 3 +- .../workflows/function/test_wiring.py | 29 +++++++++++++++++-- 3 files changed, 30 insertions(+), 4 deletions(-) diff --git a/tests/specify_cli/workflows/function/test_command_remove.py b/tests/specify_cli/workflows/function/test_command_remove.py index a1fafa4c59..b58a3820e7 100644 --- a/tests/specify_cli/workflows/function/test_command_remove.py +++ b/tests/specify_cli/workflows/function/test_command_remove.py @@ -77,7 +77,7 @@ def test_filesystem_failure_restores_the_registry_entry(self, cli, project_dir, target = install_dir(project_dir, "text") def fail(path, *args, **kwargs): - if os.fspath(path) == os.fspath(target): + if os.fspath(path) == os.fspath(target.resolve()): raise OSError("busy") return real(path, *args, **kwargs) diff --git a/tests/specify_cli/workflows/function/test_registry_parity.py b/tests/specify_cli/workflows/function/test_registry_parity.py index 6aa320d1b2..ba3b8545ad 100644 --- a/tests/specify_cli/workflows/function/test_registry_parity.py +++ b/tests/specify_cli/workflows/function/test_registry_parity.py @@ -8,6 +8,7 @@ import json import os import stat +import sys import pytest @@ -137,7 +138,7 @@ def test_save_leaves_only_the_registry_file(kind): assert [p.name for p in kind.base.iterdir()] == [kind.path.name] -@pytest.mark.skipif(not hasattr(os, "chmod"), reason="chmod is unavailable") +@pytest.mark.skipif(sys.platform == "win32", reason="chmod mode bits not reliable on Windows") def test_save_preserves_existing_file_mode(kind): registry = kind.cls(kind.root) registry.add("one", {}) diff --git a/tests/specify_cli/workflows/function/test_wiring.py b/tests/specify_cli/workflows/function/test_wiring.py index f9499578a0..648d7ab26f 100644 --- a/tests/specify_cli/workflows/function/test_wiring.py +++ b/tests/specify_cli/workflows/function/test_wiring.py @@ -3,6 +3,7 @@ from __future__ import annotations import json +import sys import pytest import yaml @@ -20,6 +21,18 @@ def custom_filters(): return sorted(set(FILTER_TABLE) - set(BUILTIN_FILTERS)) +def python_run(project_dir, name, body, *args): + """A shell ``run`` executing *body* with this interpreter. + + The shell step runs through the platform shell (``cmd.exe`` on Windows), so + fixtures avoid POSIX utilities and ``;`` chaining and use double quotes only. + """ + script = project_dir / name + script.write_text(body, encoding="utf-8") + quoted = "".join(f' "{arg}"' for arg in args) + return f'"{sys.executable}" "{script}"{quoted}' + + def workflow_yaml(steps, workflow_id="fn-wf", inputs=None): data = { "schema_version": "1.0", @@ -43,7 +56,14 @@ def trim_wf(project_dir): path = project_dir / "wf.yml" path.write_text( workflow_yaml( - [{"id": "w", "type": "shell", "run": "printf '%s' '{{ inputs.s | trim }}' > out.txt"}], + [{ + "id": "w", "type": "shell", + "run": python_run( + project_dir, "write_out.py", + "import sys\nopen('out.txt', 'w').write(sys.argv[1])\n", + "{{ inputs.s | trim }}", + ), + }], inputs={"s": {"type": "string", "default": " hi "}}, ), encoding="utf-8", @@ -115,7 +135,12 @@ def failing_wf(self, project_dir): workflow_yaml( [ {"id": "done", "type": "shell", "run": "echo {{ inputs.s | mark }}"}, - {"id": "broken", "type": "shell", "run": "echo {{ inputs.s | mark }}; exit 1"}, + { + "id": "broken", "type": "shell", + "run": python_run( + project_dir, "fail.py", "raise SystemExit(1)\n", "{{ inputs.s | mark }}" + ), + }, ], inputs={"s": {"type": "string", "default": "v"}}, ), From f9ae2ae868e40327991df7477f9860c107848dc5 Mon Sep 17 00:00:00 2001 From: Markus Date: Sat, 7 Nov 2026 13:10:53 +0100 Subject: [PATCH 09/11] test(workflows): clarify bare-condition validation test, cover wrapped loaded filter The existing validation test asserted an error after the filter was loaded, which is the bare-condition wrapping remediation, not a registry rejection; rename it accordingly and add the wrapped counterpart that must validate clean. Addresses review feedback on #5303. Assisted-by: pi coding agent (GitHub Copilot, model: claude-sonnet-5.5, autonomous) --- .../workflows/function/test_custom_filters.py | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/tests/specify_cli/workflows/function/test_custom_filters.py b/tests/specify_cli/workflows/function/test_custom_filters.py index 6541cd55c4..f09b0296be 100644 --- a/tests/specify_cli/workflows/function/test_custom_filters.py +++ b/tests/specify_cli/workflows/function/test_custom_filters.py @@ -295,7 +295,8 @@ def test_check_and_evaluation_agree_on_every_call_form(self): raised = True assert rejected == raised, form - def test_validate_workflow_reports_unknown_filter_then_accepts_loaded(self): + def test_bare_condition_error_names_unknown_filter_then_asks_for_wrapping(self): + """A bare condition always errors: first naming the filter, once loaded asking for ``{{ }}``.""" from specify_cli.workflows.engine import WorkflowDefinition, validate_workflow definition = WorkflowDefinition.from_string( @@ -321,6 +322,27 @@ def test_validate_workflow_reports_unknown_filter_then_accepts_loaded(self): assert "unknown filter" not in errors[0] assert '"{{ inputs.s | blank }}"' in errors[0] + def test_wrapped_condition_with_loaded_filter_validates_clean(self): + from specify_cli.workflows.engine import WorkflowDefinition, validate_workflow + + reg("blank", lambda v: not v) + definition = WorkflowDefinition.from_string( + """ +schema_version: "1.0" +workflow: {id: w, name: W, version: "1.0.0"} +inputs: {s: {type: string, default: x}} +steps: + - id: a + type: if + condition: "{{ inputs.s | blank }}" + then: + - id: b + type: shell + run: "echo hi" +""" + ) + assert validate_workflow(definition) == [] + # -- Handler adapter (D7) ------------------------------------------------------ From 70ce354094e4a174cf45f4cdbf92959e577b4a08 Mon Sep 17 00:00:00 2001 From: Markus Date: Sat, 7 Nov 2026 17:33:20 +0100 Subject: [PATCH 10/11] fix(workflows): keep the zero-argument filter diagnostic in condition validation _check_filter lost the "expected '| name' with no arguments or trailing tokens" message when it moved to call_form_ok, so `from_json()` and `upper()` were reported as a generic unsupported form. Restore the message and assert it in full. Refs #5285 Assisted-by: pi coding agent (GitHub Copilot, model: claude-sonnet-5.5, autonomous) --- src/specify_cli/workflows/expressions/engine.py | 3 +++ tests/unit/test_condition_expression_block.py | 3 ++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/workflows/expressions/engine.py b/src/specify_cli/workflows/expressions/engine.py index 9c9bb48e61..46dc758eb5 100644 --- a/src/specify_cli/workflows/expressions/engine.py +++ b/src/specify_cli/workflows/expressions/engine.py @@ -577,6 +577,9 @@ def _check_filter(filt: tuple[str, str, tuple[Any, ...], bool]) -> str | None: spec = FILTER_TABLE.get(name) if spec is None: return f"unknown filter '{name}'" + if has_parens and spec.max_args == 0: + # Same wording as the evaluator's zero-argument branch in `_apply_filter`. + return f"{name}: expected '| {name}' with no arguments or trailing tokens" if call_form_ok(spec, len(args), has_parens): return None return f"filter '{name}' used in an unsupported form" diff --git a/tests/unit/test_condition_expression_block.py b/tests/unit/test_condition_expression_block.py index 976143bb3a..e4d3e2934e 100644 --- a/tests/unit/test_condition_expression_block.py +++ b/tests/unit/test_condition_expression_block.py @@ -668,7 +668,8 @@ def test_a_parenthesized_or_whole_filter_chain_still_evaluates(expr, value): ("inputs.count+1", "unexpected '+1' at column 13"), ("inputs.a === inputs.b", "unexpected character '=' at column 12"), ("bogus == 'x'", "is not one of the namespace roots"), - ("inputs.payload | from_json()", "the evaluator rejects it"), + ("inputs.payload | from_json()", "from_json: expected '| from_json' with no arguments or trailing tokens"), + ("inputs.payload | upper()", "upper: expected '| upper' with no arguments or trailing tokens"), # An operator after a filter chain has both operands: it is a filter # precedence mistake, not a missing operand. A doubled `|` or operator is # the missing operand. From 138d97d9a24ba7407206a1996f354f0827db5040 Mon Sep 17 00:00:00 2001 From: Markus Date: Sat, 7 Nov 2026 17:39:17 +0100 Subject: [PATCH 11/11] fix(workflows): reject case variants of reserved function ids On a case-insensitive filesystem `FUNCTION-REGISTRY.JSON` aliases the registry file, so install published a package directory where the registry must be written and then failed on save. Compare the id case-insensitively, as validate_step_id does for step ids. Refs #5285 Assisted-by: pi coding agent (GitHub Copilot, model: claude-sonnet-5.5, autonomous) --- src/specify_cli/workflows/function/_domain.py | 2 +- tests/specify_cli/workflows/function/test_domain.py | 12 +++++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/workflows/function/_domain.py b/src/specify_cli/workflows/function/_domain.py index 3cb93f3e62..94b15bb989 100644 --- a/src/specify_cli/workflows/function/_domain.py +++ b/src/specify_cli/workflows/function/_domain.py @@ -50,7 +50,7 @@ def _validate_function_id(value: str) -> None: validate_step_id(value) except StepInstallError as exc: raise WorkflowFunctionError(str(exc).replace("step id", "function id")) from exc - if value in _RESERVED_FUNCTION_IDS: + if value.lower() in _RESERVED_FUNCTION_IDS: raise WorkflowFunctionError(f"Invalid function id '{value}': reserved name") diff --git a/tests/specify_cli/workflows/function/test_domain.py b/tests/specify_cli/workflows/function/test_domain.py index 07a32556f0..cd6c757dcd 100644 --- a/tests/specify_cli/workflows/function/test_domain.py +++ b/tests/specify_cli/workflows/function/test_domain.py @@ -178,8 +178,18 @@ def test_ids_rejected_by_the_shared_step_validator_are_rejected(self, value): with pytest.raises(WorkflowFunctionError, match="function id"): _validate_function_id(value) - @pytest.mark.parametrize("value", [".cache", "function-registry.json"]) + @pytest.mark.parametrize( + "value", + [ + ".cache", + ".CACHE", + "function-registry.json", + "Function-Registry.Json", + "FUNCTION-REGISTRY.JSON", + ], + ) def test_function_specific_reserved_names(self, value): + # Case variants alias the registry file on case-insensitive filesystems. with pytest.raises(WorkflowFunctionError): _validate_function_id(value)