From 29fd0778fc59d7da4867bf36602a4c8593b1eb7d Mon Sep 17 00:00:00 2001 From: Quratulain-bilal Date: Sun, 27 Sep 2026 19:31:55 +0500 Subject: [PATCH 1/8] feat(workflows): add upper, lower, split, length, and to_json filters Option A of #4614: the smallest high-value batch of expression filters (length, split, upper, lower, to_json), leaving first, last, sort, unique, and trim deferred until real usage justifies their semantics. Each filter validates its own input types and raises ValueError naming the problem rather than coercing, matching the strict argument handling already used by join/map/from_json. Coercion is rejected because a type mismatch almost always means the workflow is wired to the wrong variable, and a coerced result (e.g. "3" for an int) would hide that; it would also leak a cryptic TypeError/AttributeError past the evaluator, which the engine does not wrap in a try/except, crashing the whole run. - upper/lower: str -> str, empty string valid, non-str -> ValueError - split: str -> list[str], single split(sep) form only, "" -> [""], both value and separator must be strings - length: list and str -> int (Jinja2 behavior), mappings rejected so the same expression cannot mean key count for one shape and value count for another, empty -> 0, other types -> ValueError - to_json: any serializable value via json.dumps(..., sort_keys=True, ensure_ascii=False), None -> "null", non-serializable -> ValueError to_json pins sort_keys and ensure_ascii so output is byte-stable regardless of dict insertion order or platform, which is what makes a to_json -> shell step -> from_json round trip safe. The strict no-argument branch is generalized into _ZERO_ARG_FILTERS so from_json, upper, lower, length, and to_json share it; mis-wired forms (upper('x'), length extra, to_json()) raise naming the filter instead of falling through to the unknown-filter path. Existing tests that used length/upper as examples of unknown filters are retargeted at a genuinely unknown name rather than deleted, and test_registered_filters_unaffected now asserts all ten registered filters. Behavior is documented in workflows/README.md, workflows/ARCHITECTURE.md, and docs/reference/workflows.md. Note: the issue's claim that length unlocks `{% if items | length > 0 %}` does not hold. There is no `{% if %}` syntax in the evaluator, and the pipe binds tighter than comparisons, so a trailing comparison after any filter raises (pre-existing design, same as `count | default(0) > 5`). Docs instead show condition: "{{ items | length }}" (0 is falsy). Closes #4614 Assisted-by: opencode (model: mimo-v2.6-flash-free, autonomous) --- docs/reference/workflows.md | 27 +- src/specify_cli/workflows/expressions.py | 140 +++++++++- .../workflows/step/switch/__init__.py | 4 +- tests/test_workflows.py | 250 +++++++++++++++++- tests/unit/test_condition_expression_block.py | 16 +- workflows/ARCHITECTURE.md | 15 ++ workflows/README.md | 11 +- 7 files changed, 438 insertions(+), 25 deletions(-) diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index a547a10e42..31372d6962 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -590,7 +590,30 @@ Steps can reference inputs and previous step outputs using `{{ expression }}` sy | `context.run_id` | Current workflow run ID | | `context.workflow_dir` | Resolved absolute path to the workflow source directory. Empty string for string-loaded workflows. | -Available filters: `default`, `join`, `contains`, `map`, `from_json`. +Available filters: `default`, `join`, `contains`, `map`, `from_json`, `to_json`, `upper`, `lower`, `split`, `length`. + +| Filter | Example | Behavior | +| -------- | ------------------------------------------ | ----------------------------------------------------------------------------------------------- | +| `default`| `{{ val \| default('fb') }}` | Fallback for `None`/empty values | +| `join` | `{{ list \| join(', ') }}` | Join list elements into a string | +| `contains`| `{{ text \| contains('sub') }}` | Substring or membership check | +| `map` | `{{ list \| map('attr') }}` | Extract an attribute from each item | +| `from_json`| `{{ out \| from_json }}` | Parse a JSON string into a typed value | +| `to_json`| `{{ obj \| to_json }}` | Serialize a value to a JSON string — the inverse of `from_json` | +| `upper` | `{{ text \| upper }}` | Uppercase a string | +| `lower` | `{{ text \| lower }}` | Lowercase a string | +| `split` | `{{ csv \| split(',') }}` | Split a string on a separator into a list of strings | +| `length` | `{{ items \| length }}` | Number of elements in a list, or characters in a string | + +Notes on the newer filters: + +- **Types are validated, not coerced.** `upper` and `lower` accept strings only, `split` requires both a string value and a string separator, and `length` accepts lists and strings but rejects mappings. Anything else raises a `ValueError` naming the problem. Coercion is deliberately not performed: a type mismatch nearly always means the workflow is wired to the wrong variable, and a coerced result would hide that. A filter given the wrong number of arguments (`| upper('x')`, `| split` with no separator) is reported as a known filter misused, which is distinct from an entirely unknown filter name. +- **`to_json` output is deterministic.** Object keys are sorted and non-ASCII characters are left as-is rather than escaped, so the same value always serializes to the same bytes. That is what makes a `to_json` → `shell` step → `from_json` round-trip safe. +- **Trailing comparisons after a filter are rejected.** The pipe binds tighter than comparison operators, so `{{ items | length > 0 }}` raises rather than evaluating. To branch on a count, use the filter's own truthiness in a `condition:`, since `length` returns `0` for an empty input: + + ```yaml + condition: "{{ inputs.items | length }}" # 0 is False, any non-zero count is True + ``` Example: @@ -598,6 +621,8 @@ Example: condition: "{{ steps.test.output.exit_code == 0 }}" args: "{{ inputs.spec }}" message: "{{ status | default('pending') }}" +tag_count: "{{ inputs.tags | split(',') | length }}" +shell_flag: "{{ inputs.branch | upper }}" ``` ### Interpolation and shell safety diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index 426f71af9b..a8966aa56d 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -9,6 +9,7 @@ import json import re +from collections.abc import Callable from contextvars import ContextVar from typing import Any @@ -23,6 +24,11 @@ "map", "contains", "from_json", + "upper", + "lower", + "split", + "length", + "to_json", ) @@ -130,6 +136,109 @@ def _filter_from_json(value: Any) -> Any: raise ValueError(f"from_json: invalid JSON: {exc}") from exc +def _filter_upper(value: Any) -> str: + """Return *value* uppercased. + + Raises ``ValueError`` on non-string input. Without the guard a non-string + value (an authoring mistake like ``| upper`` on a shell exit code) would + reach ``str.upper`` and raise a cryptic ``AttributeError`` that escapes the + evaluator and crashes the whole run, since the engine wraps neither + expression evaluation nor ``execute`` in a try/except. Silently coercing + with ``str(value).upper()`` is rejected for the same reason ``from_json`` + does not coerce: a type mismatch here means the pipeline is wired to the + wrong variable, and rendering ``"0"`` for an int hides that. + """ + if not isinstance(value, str): + raise ValueError(f"upper: expected a string value, got {type(value).__name__}") + return value.upper() + + +def _filter_lower(value: Any) -> str: + """Return *value* lowercased. + + Raises ``ValueError`` on non-string input, for the same reason and with the + same trade-off as ``upper``: a type mismatch means the pipeline is wired to + the wrong variable, and coercing would hide it. + """ + if not isinstance(value, str): + raise ValueError(f"lower: expected a string value, got {type(value).__name__}") + return value.lower() + + +def _filter_split(value: Any, separator: str) -> list[str]: + """Split *value* on *separator* into a list of strings. + + The single-argument ``split(sep)`` form is the only one supported; there is + no maxsplit, because a partial split has no obvious meaning in a workflow + expression and an unused parameter is an authoring mistake worth reporting. + + Raises ``ValueError`` when *value* is not a string or *separator* is not a + string. Without those guards a non-string argument reaches ``str.split`` and + raises a cryptic ``TypeError`` that escapes the evaluator and crashes the + whole run, mirroring the strict argument handling in ``join`` and ``map``. + """ + if not isinstance(value, str): + raise ValueError(f"split: expected a string value, got {type(value).__name__}") + if not isinstance(separator, str): + raise ValueError( + f"split: expected a string separator, got {type(separator).__name__}" + ) + return value.split(separator) + + +def _filter_length(value: Any) -> int: + """Return the length of a list or string. + + Follows Jinja2's ``length``, which also counts characters in a string. The + two supported input types are exactly ``list`` and ``str``: dicts are + excluded even though ``len()`` accepts them, because a mapping's length is + rarely what a workflow author means by ``length`` and accepting it would + make ``{{ obj | length }}`` silently return a key count for one shape and a + value count for another. Other types (notably ``bool`` and ``None``) are + authoring mistakes and raise rather than coercing to 0. + """ + if isinstance(value, (list, str)): + return len(value) + raise ValueError( + "length: expected a list or string, got " + f"{type(value).__name__} (mappings are not supported)" + ) + + +def _filter_to_json(value: Any) -> str: + """Serialize *value* to a JSON string — the inverse of ``from_json``. + + Serialization is pinned to ``sort_keys=True`` and ``ensure_ascii=False`` so + the output is byte-stable across runs, platforms, and dict insertion order. + That determinism is what lets a workflow round-trip a value through a shell + step and recover it with ``from_json``, and it is why these flags are not + left to the default: the default key order varies with insertion order and + escapes non-ASCII as ``\\uXXXX``, so the same workflow would emit different + bytes on different runs and hand shell steps mangled text. + + Raises ``ValueError`` when *value* is not JSON-serializable, chained from + the underlying error so the offending type stays visible. + """ + try: + return json.dumps(value, sort_keys=True, ensure_ascii=False) + except (TypeError, ValueError) as exc: + raise ValueError(f"to_json: value is not JSON-serializable: {exc}") from exc + + +# Filters that take no arguments and tolerate no trailing tokens. Keyed by name +# so ``_apply_filter`` can recognize a mis-wired form of any of them from the +# leading filter name alone, instead of each needing its own branch. ``default`` +# is deliberately absent: it is the one filter that is valid both bare and with +# an argument, so it is dispatched by both of the branches below. +_ZERO_ARG_FILTERS: dict[str, Callable[[Any], Any]] = { + "from_json": _filter_from_json, + "upper": _filter_upper, + "lower": _filter_lower, + "length": _filter_length, + "to_json": _filter_to_json, +} + + # -- Expression resolution ------------------------------------------------ _EXPR_PATTERN = re.compile(r"\{\{(.+?)\}\}") @@ -463,19 +572,21 @@ def _apply_filter(value: Any, filter_expr: str, namespace: dict[str, Any]) -> An silently returning *value* unchanged: a passthrough would turn a mistyped or unsupported filter into a wrong result with no signal. """ - # `from_json` is strict: it takes no arguments and tolerates no trailing - # tokens. Match on the leading filter name and require the whole filter to - # be exactly `from_json`, so every mis-wired form (`from_json()`, - # `from_json('x')`, `from_json)`, `from_json extra`) fails loudly instead of - # silently falling through to the unknown-filter path. + # Zero-argument filters are strict: they take no arguments and tolerate no + # trailing tokens. Match on the leading filter name and require the whole + # filter to be exactly that name, so every mis-wired form (`from_json()`, + # `from_json('x')`, `from_json)`, `from_json extra`, and the same for + # `upper`/`lower`/`length`/`to_json`) fails loudly instead of silently + # falling through to the unknown-filter path. leading = re.match(r"\w+", filter_expr) - if leading and leading.group(0) == "from_json": - if filter_expr != "from_json": + if leading and leading.group(0) in _ZERO_ARG_FILTERS: + fname = leading.group(0) + if filter_expr != fname: raise ValueError( - "from_json: expected '| from_json' with no arguments or " + f"{fname}: expected '| {fname}' with no arguments or " f"trailing tokens, got '| {filter_expr}'" ) - return _filter_from_json(value) + return _ZERO_ARG_FILTERS[fname](value) # Parse filter name and argument. Use fullmatch (not match) so trailing # tokens after the closing paren — e.g. a comparison/boolean operator that @@ -496,6 +607,8 @@ def _apply_filter(value: Any, filter_expr: str, namespace: dict[str, Any]) -> An return _filter_map(value, farg) if fname == "contains": return _filter_contains(value, farg) + if fname == "split": + return _filter_split(value, farg) # Filter without args if filter_expr == "default": return _filter_default(value) @@ -507,7 +620,8 @@ def _apply_filter(value: Any, filter_expr: str, namespace: dict[str, Any]) -> An name = leading.group(0) if leading else filter_expr expected = ( "expected one of default or default('x'), join('sep'), " - "map('attr'), contains('s'), or from_json" + "map('attr'), contains('s'), split('sep'), from_json, upper, " + "lower, length, or to_json" ) if name in _REGISTERED_FILTERS: raise ValueError( @@ -538,7 +652,7 @@ def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any: - Comparisons: ``==``, ``!=``, ``>``, ``<``, ``>=``, ``<=`` - Boolean operators: ``and``, ``or``, ``not`` - ``in``, ``not in`` - - Pipe filters: ``| default('...')``, ``| join(', ')``, ``| contains('...')``, ``| from_json``, ``| map('...')`` + - Pipe filters: ``| default('...')``, ``| join(', ')``, ``| contains('...')``, ``| from_json``, ``| map('...')``, ``| split(',')``, ``| upper``, ``| lower``, ``| length``, ``| to_json`` - String and numeric literals """ expr = expr.strip() @@ -831,8 +945,8 @@ def evaluate_condition(condition: str, context: Any) -> bool: # strip that trailing newline matches neither branch and falls through to # ``bool("false\n")`` -> True, silently taking an ``if`` step's ``then`` # branch (and keeping a ``while``/``do-while`` looping) on a step that - # printed "false". A workflow cannot strip it itself -- the registered - # filters are default/join/map/contains/from_json, there is no ``trim``. + # printed "false". A workflow cannot strip it itself -- no registered + # filter trims whitespace (there is no ``trim``). # ``InitStep._resolve_bool`` and the catalog readers already strip before # matching boolean text. ``bool(result)`` below still sees the raw string, # so no non-boolean text changes truthiness. diff --git a/src/specify_cli/workflows/step/switch/__init__.py b/src/specify_cli/workflows/step/switch/__init__.py index 8a2e4b343e..2421548625 100644 --- a/src/specify_cli/workflows/step/switch/__init__.py +++ b/src/specify_cli/workflows/step/switch/__init__.py @@ -29,8 +29,8 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: # ``run: echo approve`` resolves to ``"approve\n"`` and matches no # ``approve:`` case -- the switch silently falls through to ``default:`` # while still reporting COMPLETED. A workflow cannot strip it itself: - # the registered filters are default/join/map/contains/from_json, there - # is no ``trim``. ``evaluate_condition`` and ``InitStep._resolve_bool`` + # no registered filter trims whitespace (there is no ``trim``). + # ``evaluate_condition`` and ``InitStep._resolve_bool`` # already strip before matching a resolved string against declared # literals, and case keys are exactly such literals. ``expression_value`` # below still reports the raw value, so nothing downstream loses it. diff --git a/tests/test_workflows.py b/tests/test_workflows.py index e559e14a33..ba0e2ffb25 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -598,6 +598,210 @@ def test_filter_from_json_rejects_malformed_forms(self): "{{ steps.emit.output.stdout | " + bad + " }}", ctx ) + def test_filter_upper_and_lower(self): + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"s": "Hello World", "empty": "", "mixed": "aBc"}) + assert evaluate_expression("{{ inputs.s | upper }}", ctx) == "HELLO WORLD" + assert evaluate_expression("{{ inputs.s | lower }}", ctx) == "hello world" + # An empty string is valid input, not a missing value: it must come back + # empty rather than raising or falling through to `default`. + assert evaluate_expression("{{ inputs.empty | upper }}", ctx) == "" + assert evaluate_expression("{{ inputs.empty | lower }}", ctx) == "" + # Case-only transforms are no-ops on already-conforming input. + assert evaluate_expression("{{ inputs.mixed | upper }}", ctx) == "ABC" + # Non-ASCII must pass through unchanged (no transliteration/loss). + assert evaluate_expression("{{ inputs.uni | upper }}", StepContext(inputs={"uni": "café"})) == "CAFÉ" + # Filters compose left to right with the rest of the chain. + assert evaluate_expression("{{ inputs.s | upper | lower }}", ctx) == "hello world" + + def test_filter_upper_rejects_non_string(self): + # A non-string value is an authoring mistake (e.g. `| upper` on an exit + # code). It must raise a ValueError naming the problem rather than + # coerce to "3" — which would look like a plausible result and hide the + # mis-wiring — or leak AttributeError and crash the run. + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"items": ["a"], "n": 3, "flag": True, "none": None}) + for name in ("items", "n", "flag", "none"): + with pytest.raises(ValueError, match="upper: expected a string value"): + evaluate_expression("{{ inputs." + name + " | upper }}", ctx) + + def test_filter_lower_rejects_non_string(self): + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"items": ["a"], "n": 3, "flag": True, "none": None}) + for name in ("items", "n", "flag", "none"): + with pytest.raises(ValueError, match="lower: expected a string value"): + evaluate_expression("{{ inputs." + name + " | lower }}", ctx) + + def test_filter_split(self): + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext( + inputs={ + "csv": "a,b,c", + "multi": "x::y::z", + "empty": "", + "nodelim": "abc", + "trailing": "a,b,", + } + ) + assert evaluate_expression("{{ inputs.csv | split(',') }}", ctx) == ["a", "b", "c"] + assert evaluate_expression("{{ inputs.multi | split('::') }}", ctx) == ["x", "y", "z"] + # An empty string is the one-element list [''], matching str.split. It + # must not be treated as a missing value or return []. + assert evaluate_expression("{{ inputs.empty | split(',') }}", ctx) == [""] + # A separator that never occurs yields the whole string, not []. + assert evaluate_expression("{{ inputs.nodelim | split(',') }}", ctx) == ["abc"] + # Trailing empty field is preserved, so round-tripping is lossless. + assert evaluate_expression("{{ inputs.trailing | split(',') }}", ctx) == ["a", "b", ""] + # split is the inverse of join. + assert evaluate_expression("{{ inputs.csv | split(',') | join('-') }}", ctx) == "a-b-c" + + def test_filter_split_rejects_non_string_inputs(self): + # Both the value and the separator must be strings. A non-string + # separator would otherwise leak a TypeError/AttributeError from + # str.split and crash the run. + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"items": ["a", "b"], "csv": "a,b", "n": 3}) + with pytest.raises(ValueError, match="split: expected a string value"): + evaluate_expression("{{ inputs.items | split(',') }}", ctx) + with pytest.raises(ValueError, match="split: expected a string separator"): + evaluate_expression("{{ inputs.csv | split(5) }}", ctx) + + def test_filter_length(self): + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext( + inputs={ + "items": ["a", "b", "c"], + "empty_list": [], + "s": "hello", + "empty_str": "", + } + ) + assert evaluate_expression("{{ inputs.items | length }}", ctx) == 3 + assert evaluate_expression("{{ inputs.empty_list | length }}", ctx) == 0 + # Strings count characters, as in Jinja2. + assert evaluate_expression("{{ inputs.s | length }}", ctx) == 5 + assert evaluate_expression("{{ inputs.empty_str | length }}", ctx) == 0 + # Composes with the rest of the chain, which is the motivating use. + assert evaluate_expression("{{ inputs.csv_len | length }}", StepContext(inputs={"csv_len": "a,b,c"})) == 5 + assert evaluate_expression("{{ inputs.s | split('l') | length }}", ctx) == 3 + + def test_filter_length_drives_conditions(self): + # The motivating use: `length` makes an emptiness check expressible as a + # condition, because 0 is falsy and any non-zero count is truthy. + from specify_cli.workflows.expressions import evaluate_condition + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"items": ["a"], "empty": []}) + assert evaluate_condition("{{ inputs.items | length }}", ctx) is True + assert evaluate_condition("{{ inputs.empty | length }}", ctx) is False + + def test_filter_length_rejects_unsupported_types(self): + # length accepts only list and str. Mappings are excluded on purpose so + # `{{ obj | length }}` cannot silently mean "key count" for one shape and + # "value count" for another; other types are mis-wiring and must raise + # rather than coerce to 0 (which is indistinguishable from "empty"). + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext( + inputs={"obj": {"a": 1}, "n": 3, "flag": True, "none": None, "ratio": 1.5} + ) + with pytest.raises(ValueError, match="length: expected a list or string"): + evaluate_expression("{{ inputs.obj | length }}", ctx) + for name in ("n", "flag", "none", "ratio"): + with pytest.raises(ValueError, match="length: expected a list or string"): + evaluate_expression("{{ inputs." + name + " | length }}", ctx) + + def test_filter_to_json(self): + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext( + inputs={ + "obj": {"b": 2, "a": 1}, + "items": [1, 2, 3], + "s": "hi", + "n": 3, + "flag": False, + "none": None, + "uni": "café", + } + ) + # sort_keys=True pins key order, so output does not depend on dict + # insertion order and stays byte-stable across runs. + assert evaluate_expression("{{ inputs.obj | to_json }}", ctx) == '{"a": 1, "b": 2}' + assert evaluate_expression("{{ inputs.items | to_json }}", ctx) == "[1, 2, 3]" + assert evaluate_expression("{{ inputs.s | to_json }}", ctx) == '"hi"' + assert evaluate_expression("{{ inputs.n | to_json }}", ctx) == "3" + assert evaluate_expression("{{ inputs.flag | to_json }}", ctx) == "false" + assert evaluate_expression("{{ inputs.none | to_json }}", ctx) == "null" + # ensure_ascii=False keeps non-ASCII readable instead of \uXXXX-escaped, + # so shell steps receive the original text. + assert evaluate_expression("{{ inputs.uni | to_json }}", ctx) == '"café"' + + def test_filter_to_json_round_trips_from_json(self): + # to_json is the inverse of from_json, which is the point: a workflow can + # pass a structured value through a shell step and recover it. + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext( + inputs={"obj": {"items": [1, 2, 3]}}, + steps={"emit": {"output": {"stdout": '{"items": [1, 2, 3]}'}}}, + ) + assert evaluate_expression( + "{{ steps.emit.output.stdout | from_json | to_json }}", ctx + ) == '{"items": [1, 2, 3]}' + assert evaluate_expression( + "{{ inputs.obj | to_json | from_json }}", ctx + ) == {"items": [1, 2, 3]} + + def test_filter_to_json_rejects_non_serializable(self): + # A non-serializable value must raise a ValueError naming the filter, + # not leak a TypeError from json.dumps and crash the run. + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"obj": {"f": object()}, "step": object()}) + with pytest.raises(ValueError, match="to_json: value is not JSON-serializable"): + evaluate_expression("{{ inputs.obj | to_json }}", ctx) + with pytest.raises(ValueError, match="to_json: value is not JSON-serializable"): + evaluate_expression("{{ inputs.step | to_json }}", ctx) + + def test_zero_arg_filters_reject_miswired_forms(self): + # The strict no-argument branch is shared by from_json/upper/lower/ + # length/to_json. Every mis-wired form — parenthesized, accidental arg, + # or trailing garbage — must raise naming the filter, rather than + # silently falling through to the unknown-filter path. + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"s": "hi", "items": ["a"]}) + for fname in ("from_json", "upper", "lower", "length", "to_json"): + for bad in (fname + "()", fname + "('x')", fname + ")", fname + " extra"): + with pytest.raises(ValueError, match=fname + ": expected"): + evaluate_expression( + "{{ inputs.s | " + bad + " }}", ctx + ) + def test_filter_unknown_name_raises(self): # An unregistered filter name must fail loudly rather than silently # returning the unfiltered value (which hides a typo / unsupported @@ -607,8 +811,8 @@ def test_filter_unknown_name_raises(self): from specify_cli.workflows.base import StepContext ctx = StepContext(inputs={"items": [1, 2, 3]}) - with pytest.raises(ValueError, match="unknown filter 'length'"): - evaluate_expression("{{ inputs.items | length }}", ctx) + with pytest.raises(ValueError, match="unknown filter 'truncate'"): + evaluate_expression("{{ inputs.items | truncate }}", ctx) def test_filter_unknown_name_with_args_raises(self): # The unknown-filter path must also catch the `name(arg)` form, which @@ -618,8 +822,8 @@ def test_filter_unknown_name_with_args_raises(self): from specify_cli.workflows.base import StepContext ctx = StepContext(inputs={"text": "hello"}) - with pytest.raises(ValueError, match="unknown filter 'upper'"): - evaluate_expression("{{ inputs.text | upper('x') }}", ctx) + with pytest.raises(ValueError, match="unknown filter 'truncate'"): + evaluate_expression("{{ inputs.text | truncate('x') }}", ctx) def test_filter_map_non_string_attr_raises(self): # A non-string attribute (authoring mistake like `map(5)`) must raise a @@ -667,7 +871,7 @@ def test_filter_contains_non_string_arg_on_list_ok(self): assert evaluate_expression("{{ inputs.nums | contains(9) }}", ctx) is False def test_registered_filters_unaffected(self): - # Regression: all five registered filters keep working unchanged. + # Regression: every registered filter keeps working unchanged. from specify_cli.workflows.expressions import evaluate_expression from specify_cli.workflows.base import StepContext @@ -677,6 +881,8 @@ def test_registered_filters_unaffected(self): "text": "hello world", "missing": "", "rows": [{"id": "a"}, {"id": "b"}], + "csv": "a,b,c", + "obj": {"n": 1}, }, steps={"emit": {"output": {"stdout": '{"n": 1}'}}}, ) @@ -691,6 +897,31 @@ def test_registered_filters_unaffected(self): assert evaluate_expression( "{{ steps.emit.output.stdout | from_json }}", ctx ) == {"n": 1} + assert evaluate_expression("{{ inputs.text | upper }}", ctx) == "HELLO WORLD" + assert evaluate_expression("{{ inputs.text | lower }}", ctx) == "hello world" + assert evaluate_expression("{{ inputs.csv | split(',') }}", ctx) == ["a", "b", "c"] + assert evaluate_expression("{{ inputs.tags | length }}", ctx) == 3 + assert evaluate_expression("{{ inputs.obj | to_json }}", ctx) == '{"n": 1}' + + def test_registered_filter_list_covers_every_implemented_filter(self): + # _REGISTERED_FILTERS drives the "known filter used in an unsupported + # form" message, so it must stay in sync with what is implemented: a + # registered-but-unimplemented name would be advertised in the expected + # list yet raise as unknown. + from specify_cli.workflows.expressions import _REGISTERED_FILTERS + + assert set(_REGISTERED_FILTERS) == { + "default", + "join", + "map", + "contains", + "from_json", + "upper", + "lower", + "split", + "length", + "to_json", + } def test_registered_filter_unsupported_form_raises(self): # A *registered* filter used in an unsupported form (e.g. `| join` with @@ -709,6 +940,15 @@ def test_registered_filter_unsupported_form_raises(self): ValueError, match="filter 'map' used in an unsupported form" ): evaluate_expression("{{ inputs.tags | map }}", ctx) + # An arg-taking filter must not silently accept the wrong arity. + with pytest.raises( + ValueError, match="filter 'split' used in an unsupported form" + ): + evaluate_expression("{{ inputs.tags | split }}", ctx) + with pytest.raises( + ValueError, match="filter 'contains' used in an unsupported form" + ): + evaluate_expression("{{ inputs.tags | contains }}", ctx) def test_filter_call_with_trailing_tokens_fails_loudly(self): # A trailing operator/token after a filter's closing paren must not be diff --git a/tests/unit/test_condition_expression_block.py b/tests/unit/test_condition_expression_block.py index 528798d5e2..5e5716d8dc 100644 --- a/tests/unit/test_condition_expression_block.py +++ b/tests/unit/test_condition_expression_block.py @@ -553,7 +553,7 @@ def test_incomplete_operand_covers_every_operator_the_evaluator_splits_on(): ("in inputs.tags", "missing an operand"), # leading word operator ("inputs.f(]", "brackets do not balance"), # matched count, wrong types ("inputs.f(]", "brackets do not balance"), - ("inputs.items | length", "the evaluator rejects it"), + ("inputs.items | truncate", "the evaluator rejects it"), ("inputs.tags | join", "used in an unsupported form"), ('he said "hi" then left', "is not a name the evaluator can resolve"), ("inputs.count+1", "is not a valid path segment"), @@ -612,10 +612,15 @@ def test_the_probe_reports_what_the_evaluator_reports(): Asking the evaluator removes that class: any filter used under an unknown name or in an unsupported form is reported by the code that will run. """ - assert _evaluator_rejects("inputs.items | length") is not None + assert _evaluator_rejects("inputs.items | truncate") is not None assert _evaluator_rejects("inputs.tags | join") is not None assert _evaluator_rejects("inputs.tags | join(',')") is None assert _evaluator_rejects("inputs.count > 100") is None + # A registered filter used in an unsupported form is a rejection too. + assert _evaluator_rejects("inputs.items | split") is not None + # Registered filters that are wired correctly are not. + assert _evaluator_rejects("inputs.items | length") is None + assert _evaluator_rejects("inputs.tags | split(',')") is None @pytest.mark.parametrize( @@ -671,7 +676,12 @@ def test_probe_value_errors_are_not_treated_as_rejections(condition): @pytest.mark.parametrize( "condition", - ["inputs.items | length", "inputs.tags | join"], + [ + "inputs.items | truncate", + "inputs.tags | join", + "inputs.items | split", + "inputs.text | upper('x')", + ], ) def test_filter_wiring_errors_are_still_rejections(condition): """The other half: a filter named wrong or used wrong is the author's text.""" diff --git a/workflows/ARCHITECTURE.md b/workflows/ARCHITECTURE.md index 088baac228..34a23c1e92 100644 --- a/workflows/ARCHITECTURE.md +++ b/workflows/ARCHITECTURE.md @@ -123,9 +123,24 @@ Workflow definitions use Jinja2-like `{{ expression }}` syntax for dynamic value | Filter: `contains` | `{{ text \| contains('sub') }}` | Substring/membership check | | Filter: `map` | `{{ list \| map('attr') }}` | Extract attribute from each item | | Filter: `from_json` | `{{ steps.emit.output.stdout \| from_json }}` | Parse a JSON string into a typed value (raises on invalid JSON) | +| Filter: `to_json` | `{{ obj \| to_json }}` | Serialize any value to a JSON string (inverse of `from_json`) | +| Filter: `upper` | `{{ text \| upper }}` | Uppercase a string (strings only) | +| Filter: `lower` | `{{ text \| lower }}` | Lowercase a string (strings only) | +| Filter: `split` | `{{ csv \| split(',') }}` | Split a string on a separator into a list | +| Filter: `length` | `{{ items \| length }}` | Length of a list or string (mappings rejected) | **Single expressions** (`{{ expr }}` only) return typed values. **Mixed templates** (`"text {{ expr }} more"`) return interpolated strings. +**Filter argument strictness.** Every filter validates its own input types and raises `ValueError` naming the problem rather than coercing or leaking a Python `TypeError`/`AttributeError` that would crash the run. So `upper`/`lower` accept only strings, `split` requires a string value *and* a string separator, and `length` accepts only lists and strings. Coercion is deliberately rejected: a type mismatch almost always means the pipeline is wired to the wrong variable, and a coerced result (e.g. `"3"` for an int) would hide that. A filter used with the wrong arity — `| upper('x')`, `| split` with no separator, `| join` bare — is reported as a known filter misused, distinct from an entirely unknown name. + +**`to_json` output is deterministic.** It pins `sort_keys=True` and `ensure_ascii=False`, so the same value always serializes to the same bytes regardless of dict insertion order or platform. That is what makes the `to_json` → shell step → `from_json` round-trip safe, and it is why non-ASCII text is not `\uXXXX`-escaped on its way to a shell step. + +**Filters and comparisons.** The pipe binds tighter than comparison operators, and the parser splits on the top-level `|` before looking for operators. A comparison or other trailing token after a filter is therefore rejected rather than silently evaluated (`{{ items | default(0) > 5 }}` raises) — the same holds for the new filters. To branch on a count, compare it before filtering, or test the filtered value's truthiness directly in a `condition:`, since `length` returns `0` for an empty input: + +```yaml +condition: "{{ inputs.items | length }}" # 0 -> False, any non-zero count -> True +``` + ### Namespace The expression evaluator builds a namespace from the `StepContext`: diff --git a/workflows/README.md b/workflows/README.md index 7382d6e625..904ff8c102 100644 --- a/workflows/README.md +++ b/workflows/README.md @@ -406,9 +406,18 @@ condition: "{{ steps.run-tests.output.exit_code != 0 }}" # Filters message: "{{ status | default('pending') }}" +ids: "{{ rows | split(',') | length }}" ``` -Supported filters: `default`, `join`, `contains`, `map`, `from_json`. +Supported filters: `default`, `join`, `contains`, `map`, `from_json`, `to_json`, `upper`, `lower`, `split`, `length`. + +Each filter validates its input types and raises a `ValueError` naming the +problem instead of coercing — so `upper`/`lower` take strings only, `split` +takes a string value and a string separator, and `length` accepts lists and +strings but rejects mappings. A filter used with the wrong number of arguments +(`| upper('x')`, bare `| split`) is reported as a known filter misused, which is +distinct from an entirely unknown filter name. See +[ARCHITECTURE.md](ARCHITECTURE.md#expression-evaluation) for the full table. ### Runtime Context From cc70b383f8d9549f48082c27a30ac6ed96587e9b Mon Sep 17 00:00:00 2001 From: Quratulain-bilal Date: Tue, 29 Sep 2026 02:52:30 +0500 Subject: [PATCH 2/8] fix(workflows): address Copilot review feedback on the new filters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven findings from the Copilot review of #4766, addressed in code, tests, and docs. Define and test empty-separator errors: `split('')` reached str.split and raised the bare `ValueError: empty separator`, naming neither the filter nor the expression, and no test covered it. An empty separator has no meaning in Python, so the filter now rejects it with a filter-specific message alongside the existing value and separator type checks. Scope strictness claims to the new filters: the docs said "every filter" validates its inputs, which the older filters do not — `join` stringifies unsupported shapes and elements, and `map`/`contains` return fallbacks rather than raising. README and ARCHITECTURE now state the contract for the five new filters and explicitly note that the older ones are unchanged, so authors are not promised behavior the evaluator does not provide. Replace shell-safety claims with reproducibility: determinism from `sort_keys`/`ensure_ascii` was described as making a `to_json` -> shell -> from_json round trip "safe", which contradicts the repository's shell safety model — interpolation adds no quoting or escaping, so JSON quotes and metacharacters are still interpreted by the shell. All three locations now claim reproducibility only and point at the interpolation-and-shell-safety guidance. Remove unsupported comparison guidance: "compare it before filtering" was not a usable alternative, because the count does not exist until `length` runs. The docs now say plainly that `{{ items | length > 0 }}` is unreachable and that truthiness in a `condition:` is the supported form. Assisted-by: opencode (model: mimo-v2.6-flash-free, autonomous) --- docs/reference/workflows.md | 6 ++--- src/specify_cli/workflows/expressions.py | 29 ++++++++++++++++-------- tests/test_workflows.py | 13 +++++++++++ workflows/ARCHITECTURE.md | 8 ++++--- workflows/README.md | 13 +++++++---- 5 files changed, 49 insertions(+), 20 deletions(-) diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index 31372d6962..d866373615 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -607,9 +607,9 @@ Available filters: `default`, `join`, `contains`, `map`, `from_json`, `to_json`, Notes on the newer filters: -- **Types are validated, not coerced.** `upper` and `lower` accept strings only, `split` requires both a string value and a string separator, and `length` accepts lists and strings but rejects mappings. Anything else raises a `ValueError` naming the problem. Coercion is deliberately not performed: a type mismatch nearly always means the workflow is wired to the wrong variable, and a coerced result would hide that. A filter given the wrong number of arguments (`| upper('x')`, `| split` with no separator) is reported as a known filter misused, which is distinct from an entirely unknown filter name. -- **`to_json` output is deterministic.** Object keys are sorted and non-ASCII characters are left as-is rather than escaped, so the same value always serializes to the same bytes. That is what makes a `to_json` → `shell` step → `from_json` round-trip safe. -- **Trailing comparisons after a filter are rejected.** The pipe binds tighter than comparison operators, so `{{ items | length > 0 }}` raises rather than evaluating. To branch on a count, use the filter's own truthiness in a `condition:`, since `length` returns `0` for an empty input: +- **Types are validated, not coerced.** `upper` and `lower` accept strings only, `split` requires both a string value and a non-empty string separator, and `length` accepts lists and strings but rejects mappings. Anything else raises a `ValueError` naming the problem. Coercion is deliberately not performed: a type mismatch nearly always means the workflow is wired to the wrong variable, and a coerced result would hide that. A filter given the wrong number of arguments (`| upper('x')`, `| split` with no separator) is reported as a known filter misused, which is distinct from an entirely unknown filter name. The older filters (`join`, `map`, `contains`) are more permissive and unchanged: `join` stringifies unsupported values and `map`/`contains` return fallbacks rather than raising. +- **`to_json` output is deterministic.** Object keys are sorted and non-ASCII characters are left as-is rather than escaped, so the same value always serializes to the same bytes. That buys reproducibility only — it does **not** make the result safe to pass through a shell, because [interpolation adds no quoting or escaping](#interpolation-and-shell-safety). Do not interpolate unconstrained JSON into a `run` field. +- **Trailing comparisons after a filter are rejected.** The parser splits on the top-level `|` before looking for operators, so `{{ items | length > 0 }}` raises rather than evaluating. The count does not exist until `length` runs, so there is no way to write that comparison; the supported branching form is the filter's own truthiness in a `condition:`, since `length` returns `0` for an empty input: ```yaml condition: "{{ inputs.items | length }}" # 0 is False, any non-zero count is True diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index a8966aa56d..cf7e0ed8ee 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -172,10 +172,14 @@ def _filter_split(value: Any, separator: str) -> list[str]: no maxsplit, because a partial split has no obvious meaning in a workflow expression and an unused parameter is an authoring mistake worth reporting. - Raises ``ValueError`` when *value* is not a string or *separator* is not a - string. Without those guards a non-string argument reaches ``str.split`` and - raises a cryptic ``TypeError`` that escapes the evaluator and crashes the - whole run, mirroring the strict argument handling in ``join`` and ``map``. + Raises ``ValueError`` when *value* is not a string, *separator* is not a + string, or *separator* is empty. Without those guards a non-string argument + reaches ``str.split`` and raises a cryptic ``TypeError``, and an empty + separator raises the bare ``ValueError: empty separator`` — neither names + the filter, and both escape the evaluator and crash the whole run, mirroring + the strict argument handling in ``join`` and ``map``. An empty separator has + no meaning anyway: ``str.split("")`` is an error in Python, so it is an + authoring mistake rather than a valid edge case. """ if not isinstance(value, str): raise ValueError(f"split: expected a string value, got {type(value).__name__}") @@ -183,6 +187,8 @@ def _filter_split(value: Any, separator: str) -> list[str]: raise ValueError( f"split: expected a string separator, got {type(separator).__name__}" ) + if separator == "": + raise ValueError("split: separator must not be empty") return value.split(separator) @@ -210,11 +216,16 @@ def _filter_to_json(value: Any) -> str: Serialization is pinned to ``sort_keys=True`` and ``ensure_ascii=False`` so the output is byte-stable across runs, platforms, and dict insertion order. - That determinism is what lets a workflow round-trip a value through a shell - step and recover it with ``from_json``, and it is why these flags are not - left to the default: the default key order varies with insertion order and - escapes non-ASCII as ``\\uXXXX``, so the same workflow would emit different - bytes on different runs and hand shell steps mangled text. + It is why these flags are not left to the default: the default key order + varies with insertion order and escapes non-ASCII as ``\\uXXXX``, so the + same workflow would emit different bytes on different runs and hand + downstream tools mangled text. Determinism buys *reproducibility* — the same + value always serializes identically — and nothing more. It does not make the + result safe to pass through a shell: expression interpolation adds no + quoting or escaping, so JSON quotes and metacharacters are still interpreted + by whatever runs the ``run`` field. Interpolate unconstrained JSON into a + shell step only when you have constrained what it can contain; see the + "Interpolation and shell safety" section of ``docs/reference/workflows.md``. Raises ``ValueError`` when *value* is not JSON-serializable, chained from the underlying error so the offending type stays visible. diff --git a/tests/test_workflows.py b/tests/test_workflows.py index ba0e2ffb25..13a7e2a2e2 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -679,6 +679,19 @@ def test_filter_split_rejects_non_string_inputs(self): with pytest.raises(ValueError, match="split: expected a string separator"): evaluate_expression("{{ inputs.csv | split(5) }}", ctx) + def test_filter_split_rejects_empty_separator(self): + # `str.split("")` raises the bare `ValueError: empty separator`, which + # names neither the filter nor the expression and escapes the evaluator + # as a raw Python error. An empty separator has no meaning, so it must + # be reported by the filter itself like every other misuse. + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"csv": "a,b"}) + with pytest.raises(ValueError, match="split: separator must not be empty"): + evaluate_expression("{{ inputs.csv | split('') }}", ctx) + def test_filter_length(self): from specify_cli.workflows.expressions import evaluate_expression from specify_cli.workflows.base import StepContext diff --git a/workflows/ARCHITECTURE.md b/workflows/ARCHITECTURE.md index 34a23c1e92..37c0919b13 100644 --- a/workflows/ARCHITECTURE.md +++ b/workflows/ARCHITECTURE.md @@ -131,11 +131,13 @@ Workflow definitions use Jinja2-like `{{ expression }}` syntax for dynamic value **Single expressions** (`{{ expr }}` only) return typed values. **Mixed templates** (`"text {{ expr }} more"`) return interpolated strings. -**Filter argument strictness.** Every filter validates its own input types and raises `ValueError` naming the problem rather than coercing or leaking a Python `TypeError`/`AttributeError` that would crash the run. So `upper`/`lower` accept only strings, `split` requires a string value *and* a string separator, and `length` accepts only lists and strings. Coercion is deliberately rejected: a type mismatch almost always means the pipeline is wired to the wrong variable, and a coerced result (e.g. `"3"` for an int) would hide that. A filter used with the wrong arity — `| upper('x')`, `| split` with no separator, `| join` bare — is reported as a known filter misused, distinct from an entirely unknown name. +**Filter argument strictness (new filters).** The five filters added in #4766 validate their supported inputs and raise `ValueError` naming the problem rather than leaking a Python `TypeError`/`AttributeError`: `upper`/`lower` accept only strings, `split` requires a string value and a non-empty string separator, `length` accepts only lists and strings, and `to_json` requires a JSON-serializable value. Coercion is deliberately rejected for these filters: a type mismatch almost always means the pipeline is wired to the wrong variable, and a coerced result (e.g. `"3"` for an int) would hide that. A filter used with the wrong arity — `| upper('x')`, `| split` with no separator, `| join` bare — is reported as a known filter misused, distinct from an entirely unknown name. -**`to_json` output is deterministic.** It pins `sort_keys=True` and `ensure_ascii=False`, so the same value always serializes to the same bytes regardless of dict insertion order or platform. That is what makes the `to_json` → shell step → `from_json` round-trip safe, and it is why non-ASCII text is not `\uXXXX`-escaped on its way to a shell step. +The older filters are more permissive and are unchanged: `join` stringifies unsupported value shapes and elements, and `map`/`contains` return fallback values for unsupported inputs rather than raising. -**Filters and comparisons.** The pipe binds tighter than comparison operators, and the parser splits on the top-level `|` before looking for operators. A comparison or other trailing token after a filter is therefore rejected rather than silently evaluated (`{{ items | default(0) > 5 }}` raises) — the same holds for the new filters. To branch on a count, compare it before filtering, or test the filtered value's truthiness directly in a `condition:`, since `length` returns `0` for an empty input: +**`to_json` output is deterministic.** It pins `sort_keys=True` and `ensure_ascii=False`, so the same value always serializes to the same bytes regardless of dict insertion order or platform, and non-ASCII text is not `\uXXXX`-escaped. This buys reproducibility only — it does **not** make the result safe to pass through a shell, because expression interpolation adds no quoting or escaping. See [Interpolation and shell safety](../docs/reference/workflows.md#interpolation-and-shell-safety) before interpolating JSON into a `run` field. + +**Filters and comparisons.** The parser splits on the top-level `|` before looking for operators, so a comparison or other trailing token after a filter is rejected as ambiguous rather than silently evaluated (`{{ items | default(0) > 5 }}` raises) — the same holds for the new filters. The count a `length` filter would produce does not exist before the filter runs, so there is no way to write `{{ items | length > 0 }}`; the only supported branching form is the filtered value's own truthiness in a `condition:`, since `length` returns `0` for an empty input: ```yaml condition: "{{ inputs.items | length }}" # 0 -> False, any non-zero count -> True diff --git a/workflows/README.md b/workflows/README.md index 904ff8c102..f47370eb63 100644 --- a/workflows/README.md +++ b/workflows/README.md @@ -411,12 +411,15 @@ ids: "{{ rows | split(',') | length }}" Supported filters: `default`, `join`, `contains`, `map`, `from_json`, `to_json`, `upper`, `lower`, `split`, `length`. -Each filter validates its input types and raises a `ValueError` naming the -problem instead of coercing — so `upper`/`lower` take strings only, `split` -takes a string value and a string separator, and `length` accepts lists and -strings but rejects mappings. A filter used with the wrong number of arguments +The new filters validate their input types and raise a `ValueError` naming +the problem instead of coercing — so `upper`/`lower` take strings only, `split` +takes a string value and a non-empty string separator, and `length` accepts +lists and strings but rejects mappings. A filter used with the wrong number of +arguments (`| upper('x')`, bare `| split`) is reported as a known filter misused, which is -distinct from an entirely unknown filter name. See +distinct from an entirely unknown filter name. The older filters are more +permissive and unchanged: `join` stringifies unsupported values, and +`map`/`contains` return fallbacks rather than raising. See [ARCHITECTURE.md](ARCHITECTURE.md#expression-evaluation) for the full table. ### Runtime Context From 51b4049b4dd3d6ffe459d6e9b90e3da8c5fcba64 Mon Sep 17 00:00:00 2001 From: Quratulain-bilal Date: Tue, 29 Sep 2026 03:12:46 +0500 Subject: [PATCH 3/8] test(workflows): limit to_json round-trip claim to the evaluator The test comment promised a structured value could pass through a shell step and be recovered, which the evaluator does not guarantee: interpolation adds no quoting, so ShellStep would hand the JSON to shell=True unescaped. The test only exercises to_json/from_json inside the evaluator, so say that and point at the interpolation-and-shell-safety section instead of promising a shell round trip. Assisted-by: opencode (model: mimo-v2.6-flash-free, autonomous) --- tests/test_workflows.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 13a7e2a2e2..f650102d6e 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -769,8 +769,11 @@ def test_filter_to_json(self): assert evaluate_expression("{{ inputs.uni | to_json }}", ctx) == '"café"' def test_filter_to_json_round_trips_from_json(self): - # to_json is the inverse of from_json, which is the point: a workflow can - # pass a structured value through a shell step and recover it. + # to_json is the inverse of from_json: a structured value survives an + # evaluator-level round trip. Nothing here exercises a shell, and the + # round trip is not extended to one — interpolation adds no quoting, so + # the output is reproducible but not shell-safe (see docs/reference/ + # workflows.md, "Interpolation and shell safety"). from specify_cli.workflows.expressions import evaluate_expression from specify_cli.workflows.base import StepContext From 64d12171a9a704ae1c7490f29eaa2bc89d83a9da Mon Sep 17 00:00:00 2001 From: Quratulain-bilal Date: Tue, 29 Sep 2026 17:42:23 +0500 Subject: [PATCH 4/8] fix(workflows): reject non-finite floats in to_json json.dumps defaults to allow_nan=True, so to_json emitted bare NaN, Infinity, and -Infinity. None of those is valid JSON: the filter promised a JSON string and handed standards-compliant parsers something they must reject. Pass allow_nan=False so non-finite floats take the same ValueError path as any other unserializable value, and pin all three plus a nested case with a regression test. Documented alongside the existing to_json notes in README, ARCHITECTURE, and the reference. Assisted-by: opencode (model: mimo-v2.6-flash-free, autonomous) --- docs/reference/workflows.md | 1 + src/specify_cli/workflows/expressions.py | 7 +++++-- tests/test_workflows.py | 23 +++++++++++++++++++++++ workflows/ARCHITECTURE.md | 2 +- workflows/README.md | 7 +++++-- 5 files changed, 35 insertions(+), 5 deletions(-) diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index d866373615..b50e39f2f7 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -609,6 +609,7 @@ Notes on the newer filters: - **Types are validated, not coerced.** `upper` and `lower` accept strings only, `split` requires both a string value and a non-empty string separator, and `length` accepts lists and strings but rejects mappings. Anything else raises a `ValueError` naming the problem. Coercion is deliberately not performed: a type mismatch nearly always means the workflow is wired to the wrong variable, and a coerced result would hide that. A filter given the wrong number of arguments (`| upper('x')`, `| split` with no separator) is reported as a known filter misused, which is distinct from an entirely unknown filter name. The older filters (`join`, `map`, `contains`) are more permissive and unchanged: `join` stringifies unsupported values and `map`/`contains` return fallbacks rather than raising. - **`to_json` output is deterministic.** Object keys are sorted and non-ASCII characters are left as-is rather than escaped, so the same value always serializes to the same bytes. That buys reproducibility only — it does **not** make the result safe to pass through a shell, because [interpolation adds no quoting or escaping](#interpolation-and-shell-safety). Do not interpolate unconstrained JSON into a `run` field. +- **`to_json` rejects non-finite floats.** `NaN`, `Infinity`, and `-Infinity` raise a `ValueError` naming `to_json` instead of serializing to bare tokens. None of the three is valid JSON, so emitting them would hand a standards-compliant downstream parser a string it must reject. - **Trailing comparisons after a filter are rejected.** The parser splits on the top-level `|` before looking for operators, so `{{ items | length > 0 }}` raises rather than evaluating. The count does not exist until `length` runs, so there is no way to write that comparison; the supported branching form is the filter's own truthiness in a `condition:`, since `length` returns `0` for an empty input: ```yaml diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index cf7e0ed8ee..6a5360c7aa 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -228,10 +228,13 @@ def _filter_to_json(value: Any) -> str: "Interpolation and shell safety" section of ``docs/reference/workflows.md``. Raises ``ValueError`` when *value* is not JSON-serializable, chained from - the underlying error so the offending type stays visible. + the underlying error so the offending type stays visible. ``allow_nan=False`` + is what makes that true for non-finite floats: ``json.dumps`` would + otherwise emit bare ``NaN``/``Infinity``/``-Infinity``, none of which is + valid JSON, and hand downstream parsers a string they must reject. """ try: - return json.dumps(value, sort_keys=True, ensure_ascii=False) + return json.dumps(value, sort_keys=True, ensure_ascii=False, allow_nan=False) except (TypeError, ValueError) as exc: raise ValueError(f"to_json: value is not JSON-serializable: {exc}") from exc diff --git a/tests/test_workflows.py b/tests/test_workflows.py index f650102d6e..9d05c6a58b 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -801,6 +801,29 @@ def test_filter_to_json_rejects_non_serializable(self): with pytest.raises(ValueError, match="to_json: value is not JSON-serializable"): evaluate_expression("{{ inputs.step | to_json }}", ctx) + def test_filter_to_json_rejects_non_finite_floats(self): + # json.dumps defaults to allow_nan=True, which would emit bare + # NaN/Infinity/-Infinity — none of them valid JSON. All three must + # take the ValueError path instead, both at the top level and when + # they are buried inside a container. + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext( + inputs={ + "nan": float("nan"), + "inf": float("inf"), + "ninf": float("-inf"), + "nested": [float("nan")], + } + ) + for name in ("nan", "inf", "ninf", "nested"): + with pytest.raises( + ValueError, match="to_json: value is not JSON-serializable" + ): + evaluate_expression(f"{{{{ inputs.{name} | to_json }}}}", ctx) + def test_zero_arg_filters_reject_miswired_forms(self): # The strict no-argument branch is shared by from_json/upper/lower/ # length/to_json. Every mis-wired form — parenthesized, accidental arg, diff --git a/workflows/ARCHITECTURE.md b/workflows/ARCHITECTURE.md index 37c0919b13..a3bbf59433 100644 --- a/workflows/ARCHITECTURE.md +++ b/workflows/ARCHITECTURE.md @@ -131,7 +131,7 @@ Workflow definitions use Jinja2-like `{{ expression }}` syntax for dynamic value **Single expressions** (`{{ expr }}` only) return typed values. **Mixed templates** (`"text {{ expr }} more"`) return interpolated strings. -**Filter argument strictness (new filters).** The five filters added in #4766 validate their supported inputs and raise `ValueError` naming the problem rather than leaking a Python `TypeError`/`AttributeError`: `upper`/`lower` accept only strings, `split` requires a string value and a non-empty string separator, `length` accepts only lists and strings, and `to_json` requires a JSON-serializable value. Coercion is deliberately rejected for these filters: a type mismatch almost always means the pipeline is wired to the wrong variable, and a coerced result (e.g. `"3"` for an int) would hide that. A filter used with the wrong arity — `| upper('x')`, `| split` with no separator, `| join` bare — is reported as a known filter misused, distinct from an entirely unknown name. +**Filter argument strictness (new filters).** The five filters added in #4766 validate their supported inputs and raise `ValueError` naming the problem rather than leaking a Python `TypeError`/`AttributeError`: `upper`/`lower` accept only strings, `split` requires a string value and a non-empty string separator, `length` accepts only lists and strings, and `to_json` requires a JSON-serializable value and additionally rejects non-finite floats (`NaN`, `Infinity`, `-Infinity`), which `json.dumps` would otherwise emit as bare tokens that are not valid JSON. Coercion is deliberately rejected for these filters: a type mismatch almost always means the pipeline is wired to the wrong variable, and a coerced result (e.g. `"3"` for an int) would hide that. A filter used with the wrong arity — `| upper('x')`, `| split` with no separator, `| join` bare — is reported as a known filter misused, distinct from an entirely unknown name. The older filters are more permissive and are unchanged: `join` stringifies unsupported value shapes and elements, and `map`/`contains` return fallback values for unsupported inputs rather than raising. diff --git a/workflows/README.md b/workflows/README.md index f47370eb63..fc08d40bb1 100644 --- a/workflows/README.md +++ b/workflows/README.md @@ -413,8 +413,11 @@ Supported filters: `default`, `join`, `contains`, `map`, `from_json`, `to_json`, The new filters validate their input types and raise a `ValueError` naming the problem instead of coercing — so `upper`/`lower` take strings only, `split` -takes a string value and a non-empty string separator, and `length` accepts -lists and strings but rejects mappings. A filter used with the wrong number of +takes a string value and a non-empty string separator, `length` accepts +lists and strings but rejects mappings, and `to_json` rejects values it +cannot serialize — including non-finite floats (`NaN`, `Infinity`, +`-Infinity`), which would otherwise serialize to tokens that are not valid +JSON. A filter used with the wrong number of arguments (`| upper('x')`, bare `| split`) is reported as a known filter misused, which is distinct from an entirely unknown filter name. The older filters are more From 31928a45b179aabc7e12dc5c712bca96c144c2eb Mon Sep 17 00:00:00 2001 From: Quratulain-bilal Date: Tue, 29 Sep 2026 19:49:30 +0500 Subject: [PATCH 5/8] fix(workflows): validate to_json keys and filter arity before evaluating Both findings from the latest review round hid the real authoring mistake behind a less useful error. to_json relied on sort_keys=True for determinism, so a mapping with mixed key types raised an ordering TypeError that the generic handler reported as "not JSON-serializable", while a mapping with a lone integer key was silently coerced to "1" and could collide with an existing "1" key. JSON objects have string keys, so keys are validated before json.dumps is reached and the message names the key type instead. The walk is iterative with a seen set, leaving circular structures for json.dumps to report. split(',', 1) evaluated the argument expression first. The evaluator has no comma operator, so the fragment evaluated to None and the reported error was "expected a string separator, got NoneType" -- the extra argument never surfaced. Arity for the single-argument filters is now counted on the top-level comma split, before evaluation, so the message names the filter and the argument count. Documented in workflows/README.md, workflows/ARCHITECTURE.md, and docs/reference/workflows.md, with a regression test for each behavior. Assisted-by: opencode (model: mimo-v2.6-flash-free, autonomous) --- docs/reference/workflows.md | 5 ++- src/specify_cli/workflows/expressions.py | 57 +++++++++++++++++++++++- tests/test_workflows.py | 53 ++++++++++++++++++++++ workflows/ARCHITECTURE.md | 4 +- workflows/README.md | 12 ++--- 5 files changed, 121 insertions(+), 10 deletions(-) diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index b50e39f2f7..16773ae7f2 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -599,7 +599,7 @@ Available filters: `default`, `join`, `contains`, `map`, `from_json`, `to_json`, | `contains`| `{{ text \| contains('sub') }}` | Substring or membership check | | `map` | `{{ list \| map('attr') }}` | Extract an attribute from each item | | `from_json`| `{{ out \| from_json }}` | Parse a JSON string into a typed value | -| `to_json`| `{{ obj \| to_json }}` | Serialize a value to a JSON string — the inverse of `from_json` | +| `to_json`| `{{ obj \| to_json }}` | Serialize a value to a JSON string — the inverse of `from_json`; mapping keys must be strings | | `upper` | `{{ text \| upper }}` | Uppercase a string | | `lower` | `{{ text \| lower }}` | Lowercase a string | | `split` | `{{ csv \| split(',') }}` | Split a string on a separator into a list of strings | @@ -607,9 +607,10 @@ Available filters: `default`, `join`, `contains`, `map`, `from_json`, `to_json`, Notes on the newer filters: -- **Types are validated, not coerced.** `upper` and `lower` accept strings only, `split` requires both a string value and a non-empty string separator, and `length` accepts lists and strings but rejects mappings. Anything else raises a `ValueError` naming the problem. Coercion is deliberately not performed: a type mismatch nearly always means the workflow is wired to the wrong variable, and a coerced result would hide that. A filter given the wrong number of arguments (`| upper('x')`, `| split` with no separator) is reported as a known filter misused, which is distinct from an entirely unknown filter name. The older filters (`join`, `map`, `contains`) are more permissive and unchanged: `join` stringifies unsupported values and `map`/`contains` return fallbacks rather than raising. +- **Types are validated, not coerced.** `upper` and `lower` accept strings only, `split` requires both a string value and a non-empty string separator, and `length` accepts lists and strings but rejects mappings. Anything else raises a `ValueError` naming the problem. Coercion is deliberately not performed: a type mismatch nearly always means the workflow is wired to the wrong variable, and a coerced result would hide that. A filter given the wrong number of arguments (`| upper('x')`, `| split` with no separator, `| split(',', 1)`) is reported as a known filter misused, which is distinct from an entirely unknown filter name; arity is checked before the argument expression is evaluated, so the extra argument is what gets named. The older filters (`join`, `map`, `contains`) are more permissive and unchanged: `join` stringifies unsupported values and `map`/`contains` return fallbacks rather than raising. - **`to_json` output is deterministic.** Object keys are sorted and non-ASCII characters are left as-is rather than escaped, so the same value always serializes to the same bytes. That buys reproducibility only — it does **not** make the result safe to pass through a shell, because [interpolation adds no quoting or escaping](#interpolation-and-shell-safety). Do not interpolate unconstrained JSON into a `run` field. - **`to_json` rejects non-finite floats.** `NaN`, `Infinity`, and `-Infinity` raise a `ValueError` naming `to_json` instead of serializing to bare tokens. None of the three is valid JSON, so emitting them would hand a standards-compliant downstream parser a string it must reject. +- **`to_json` requires string mapping keys.** JSON objects have string keys, so a mapping with any other key type — `{1: "a"}`, `{True: "a"}` — raises a `ValueError` naming the key type instead. Left to `json.dumps`, an integer key would be coerced to `"1"` and collide with an existing `"1"` key, while mixed key types would fail `sort_keys` with an ordering `TypeError` reported only as "not JSON-serializable". - **Trailing comparisons after a filter are rejected.** The parser splits on the top-level `|` before looking for operators, so `{{ items | length > 0 }}` raises rather than evaluating. The count does not exist until `length` runs, so there is no way to write that comparison; the supported branching form is the filter's own truthiness in a `condition:`, since `length` returns `0` for an empty input: ```yaml diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index 6a5360c7aa..07ed722bbf 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -232,13 +232,52 @@ def _filter_to_json(value: Any) -> str: is what makes that true for non-finite floats: ``json.dumps`` would otherwise emit bare ``NaN``/``Infinity``/``-Infinity``, none of which is valid JSON, and hand downstream parsers a string they must reject. + + Mapping keys must be strings, which is what JSON objects have anyway. + ``_check_json_keys`` enforces that before ``json.dumps`` is reached, so a + non-string key is reported as the authoring mistake it is rather than + surfacing as an ordering ``TypeError`` from ``sort_keys=True`` (mixed key + types) or as silent ``1`` → ``"1"`` coercion that collides with an existing + ``"1"`` key. """ + _check_json_keys(value) try: return json.dumps(value, sort_keys=True, ensure_ascii=False, allow_nan=False) except (TypeError, ValueError) as exc: raise ValueError(f"to_json: value is not JSON-serializable: {exc}") from exc +def _check_json_keys(value: Any) -> None: + """Raise ``ValueError`` if any mapping reachable from *value* has a + non-string key. + + Walked iteratively with a ``seen`` set: a self-referential structure is + skipped rather than recursed into, so the circular reference stays for + ``json.dumps`` to report with its own clearer message instead of the walk + exhausting the stack first. + """ + seen: set[int] = set() + stack: list[Any] = [value] + while stack: + item = stack.pop() + if isinstance(item, dict): + if id(item) in seen: + continue + seen.add(id(item)) + for key, sub_value in item.items(): + if not isinstance(key, str): + raise ValueError( + "to_json: mapping keys must be strings, got " + f"{type(key).__name__}: {key!r}" + ) + stack.append(sub_value) + elif isinstance(item, (list, tuple)): + if id(item) in seen: + continue + seen.add(id(item)) + stack.extend(item) + + # Filters that take no arguments and tolerate no trailing tokens. Keyed by name # so ``_apply_filter`` can recognize a mis-wired form of any of them from the # leading filter name alone, instead of each needing its own branch. ``default`` @@ -252,6 +291,14 @@ def _filter_to_json(value: Any) -> str: "to_json": _filter_to_json, } +# Parenthesized filters that take exactly one argument. Used to report an +# extra argument by name *before* the argument expression is evaluated: +# _evaluate_simple_expression has no comma operator, so ``| split(',', 1)`` +# would otherwise hand split an unparseable fragment, which reads back as +# "expected a string separator, got NoneType" -- the arity mistake, which is +# what the author actually got wrong, never surfaces. +_SINGLE_ARG_FILTERS = frozenset({"default", "join", "map", "contains", "split"}) + # -- Expression resolution ------------------------------------------------ @@ -612,7 +659,15 @@ def _apply_filter(value: Any, filter_expr: str, namespace: dict[str, Any]) -> An filter_match = re.fullmatch(r"(\w+)\((.+)\)", filter_expr) if filter_match: fname = filter_match.group(1) - farg = _evaluate_simple_expression(filter_match.group(2).strip(), namespace) + farg_text = filter_match.group(2).strip() + if fname in _SINGLE_ARG_FILTERS: + arg_parts = _split_top_level_commas(farg_text) + if len(arg_parts) > 1: + raise ValueError( + f"{fname}: expected exactly one argument, got " + f"{len(arg_parts)}: '| {filter_expr}'" + ) + farg = _evaluate_simple_expression(farg_text, namespace) if fname == "default": return _filter_default(value, farg) if fname == "join": diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 9d05c6a58b..37cd8db3e1 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -824,6 +824,59 @@ def test_filter_to_json_rejects_non_finite_floats(self): ): evaluate_expression(f"{{{{ inputs.{name} | to_json }}}}", ctx) + def test_filter_to_json_rejects_non_string_keys(self): + # JSON objects have string keys only. Without this check, json.dumps + # would coerce 1 to "1" -- colliding with an existing "1" key -- and + # sort_keys=True would raise an ordering TypeError on mixed key types, + # which surfaced as a generic "not JSON-serializable" hiding the real + # authoring mistake. + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext( + inputs={ + "mixed": {1: "a", "2": "b"}, + "intkey": {1: "a"}, + "nested": {"outer": {3: "deep"}}, + "strkey": {"1": "a", "2": "b"}, + } + ) + for name in ("mixed", "intkey", "nested"): + with pytest.raises( + ValueError, match="to_json: mapping keys must be strings" + ): + evaluate_expression(f"{{{{ inputs.{name} | to_json }}}}", ctx) + # String keys, digit strings included, still serialize normally. + assert ( + evaluate_expression("{{ inputs.strkey | to_json }}", ctx) + == '{"1": "a", "2": "b"}' + ) + + def test_single_arg_filters_reject_extra_argument(self): + # Arity is checked before the argument expression is evaluated. + # split(',', 1) used to evaluate that fragment first, so the reported + # error was "expected a string separator, got NoneType" -- the arity + # mistake, which is what the author got wrong, stayed hidden behind a + # type error. + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"s": "a,b,c", "n": 0}) + with pytest.raises( + ValueError, match="split: expected exactly one argument, got 2" + ): + evaluate_expression("{{ inputs.s | split(',', 1) }}", ctx) + with pytest.raises( + ValueError, match="default: expected exactly one argument, got 2" + ): + evaluate_expression("{{ inputs.n | default(0, 'x') }}", ctx) + # A comma inside quotes or brackets is still one argument. + assert ( + evaluate_expression("{{ inputs.s | split(',') }}", ctx) == ["a", "b", "c"] + ) + def test_zero_arg_filters_reject_miswired_forms(self): # The strict no-argument branch is shared by from_json/upper/lower/ # length/to_json. Every mis-wired form — parenthesized, accidental arg, diff --git a/workflows/ARCHITECTURE.md b/workflows/ARCHITECTURE.md index a3bbf59433..150cd3c200 100644 --- a/workflows/ARCHITECTURE.md +++ b/workflows/ARCHITECTURE.md @@ -123,7 +123,7 @@ Workflow definitions use Jinja2-like `{{ expression }}` syntax for dynamic value | Filter: `contains` | `{{ text \| contains('sub') }}` | Substring/membership check | | Filter: `map` | `{{ list \| map('attr') }}` | Extract attribute from each item | | Filter: `from_json` | `{{ steps.emit.output.stdout \| from_json }}` | Parse a JSON string into a typed value (raises on invalid JSON) | -| Filter: `to_json` | `{{ obj \| to_json }}` | Serialize any value to a JSON string (inverse of `from_json`) | +| Filter: `to_json` | `{{ obj \| to_json }}` | Serialize a value to a JSON string (inverse of `from_json`; mapping keys must be strings) | | Filter: `upper` | `{{ text \| upper }}` | Uppercase a string (strings only) | | Filter: `lower` | `{{ text \| lower }}` | Lowercase a string (strings only) | | Filter: `split` | `{{ csv \| split(',') }}` | Split a string on a separator into a list | @@ -131,7 +131,7 @@ Workflow definitions use Jinja2-like `{{ expression }}` syntax for dynamic value **Single expressions** (`{{ expr }}` only) return typed values. **Mixed templates** (`"text {{ expr }} more"`) return interpolated strings. -**Filter argument strictness (new filters).** The five filters added in #4766 validate their supported inputs and raise `ValueError` naming the problem rather than leaking a Python `TypeError`/`AttributeError`: `upper`/`lower` accept only strings, `split` requires a string value and a non-empty string separator, `length` accepts only lists and strings, and `to_json` requires a JSON-serializable value and additionally rejects non-finite floats (`NaN`, `Infinity`, `-Infinity`), which `json.dumps` would otherwise emit as bare tokens that are not valid JSON. Coercion is deliberately rejected for these filters: a type mismatch almost always means the pipeline is wired to the wrong variable, and a coerced result (e.g. `"3"` for an int) would hide that. A filter used with the wrong arity — `| upper('x')`, `| split` with no separator, `| join` bare — is reported as a known filter misused, distinct from an entirely unknown name. +**Filter argument strictness (new filters).** The five filters added in #4766 validate their supported inputs and raise `ValueError` naming the problem rather than leaking a Python `TypeError`/`AttributeError`: `upper`/`lower` accept only strings, `split` requires a string value and a non-empty string separator, `length` accepts only lists and strings, and `to_json` requires a JSON-serializable value whose mapping keys are strings, and additionally rejects non-finite floats (`NaN`, `Infinity`, `-Infinity`), which `json.dumps` would otherwise emit as bare tokens that are not valid JSON. Keys are checked before serialization because `json.dumps` would otherwise coerce `1` to `"1"` (colliding with an existing `"1"` key), while `sort_keys=True` raises an ordering `TypeError` on mixed key types — both surfacing as a generic "not JSON-serializable" that hides the authoring mistake. Coercion is deliberately rejected for these filters: a type mismatch almost always means the pipeline is wired to the wrong variable, and a coerced result (e.g. `"3"` for an int) would hide that. Arity is checked before the argument expression is evaluated, so a filter used with the wrong number of arguments — `| upper('x')`, `| split` with no separator, `| split(',', 1)`, `| join` bare — is reported as a known filter misused, distinct from an entirely unknown name. The older filters are more permissive and are unchanged: `join` stringifies unsupported value shapes and elements, and `map`/`contains` return fallback values for unsupported inputs rather than raising. diff --git a/workflows/README.md b/workflows/README.md index fc08d40bb1..b8c2f5bb11 100644 --- a/workflows/README.md +++ b/workflows/README.md @@ -414,12 +414,14 @@ Supported filters: `default`, `join`, `contains`, `map`, `from_json`, `to_json`, The new filters validate their input types and raise a `ValueError` naming the problem instead of coercing — so `upper`/`lower` take strings only, `split` takes a string value and a non-empty string separator, `length` accepts -lists and strings but rejects mappings, and `to_json` rejects values it -cannot serialize — including non-finite floats (`NaN`, `Infinity`, -`-Infinity`), which would otherwise serialize to tokens that are not valid -JSON. A filter used with the wrong number of +lists and strings but rejects mappings, and `to_json` rejects what it cannot +serialize: non-finite floats (`NaN`, `Infinity`, `-Infinity`), which would +otherwise serialize to tokens that are not valid JSON, and mapping keys that +are not strings, which JSON objects cannot carry. A filter used with the wrong +number of arguments -(`| upper('x')`, bare `| split`) is reported as a known filter misused, which is +(`| upper('x')`, `| split(',', 1)`, bare `| split`) is reported as a known +filter misused, which is distinct from an entirely unknown filter name. The older filters are more permissive and unchanged: `join` stringifies unsupported values, and `map`/`contains` return fallbacks rather than raising. See From ab5882c0e1be00c6a2364a0b9891befcf9963b45 Mon Sep 17 00:00:00 2001 From: Quratulain-bilal Date: Wed, 30 Sep 2026 21:16:32 +0500 Subject: [PATCH 6/8] docs(workflows): narrow the default fallback wording Both filter tables said "empty values" / "None/empty", which reads as though 0, false, [], and {} fall back too. _filter_default only checks for None and the empty string, so a zero count comes back as 0. Spell out exactly which values fall back in both tables, and say once in the reference that falsy is not the same as empty here. Assisted-by: opencode (model: mimo-v2.6-flash-free, autonomous) --- docs/reference/workflows.md | 4 +++- workflows/ARCHITECTURE.md | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index 16773ae7f2..48738ba622 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -594,7 +594,7 @@ Available filters: `default`, `join`, `contains`, `map`, `from_json`, `to_json`, | Filter | Example | Behavior | | -------- | ------------------------------------------ | ----------------------------------------------------------------------------------------------- | -| `default`| `{{ val \| default('fb') }}` | Fallback for `None`/empty values | +| `default`| `{{ val \| default('fb') }}` | Fallback for `None` or an empty string | | `join` | `{{ list \| join(', ') }}` | Join list elements into a string | | `contains`| `{{ text \| contains('sub') }}` | Substring or membership check | | `map` | `{{ list \| map('attr') }}` | Extract an attribute from each item | @@ -605,6 +605,8 @@ Available filters: `default`, `join`, `contains`, `map`, `from_json`, `to_json`, | `split` | `{{ csv \| split(',') }}` | Split a string on a separator into a list of strings | | `length` | `{{ items \| length }}` | Number of elements in a list, or characters in a string | +`default` falls back only for `None` and the empty string. Other falsy values — `0`, `false`, `[]`, `{}` — are passed through unchanged, so `{{ count | default(10) }}` still yields `0` for a zero count. Falsy is not the same as empty here. + Notes on the newer filters: - **Types are validated, not coerced.** `upper` and `lower` accept strings only, `split` requires both a string value and a non-empty string separator, and `length` accepts lists and strings but rejects mappings. Anything else raises a `ValueError` naming the problem. Coercion is deliberately not performed: a type mismatch nearly always means the workflow is wired to the wrong variable, and a coerced result would hide that. A filter given the wrong number of arguments (`| upper('x')`, `| split` with no separator, `| split(',', 1)`) is reported as a known filter misused, which is distinct from an entirely unknown filter name; arity is checked before the argument expression is evaluated, so the extra argument is what gets named. The older filters (`join`, `map`, `contains`) are more permissive and unchanged: `join` stringifies unsupported values and `map`/`contains` return fallbacks rather than raising. diff --git a/workflows/ARCHITECTURE.md b/workflows/ARCHITECTURE.md index 150cd3c200..fbe3301621 100644 --- a/workflows/ARCHITECTURE.md +++ b/workflows/ARCHITECTURE.md @@ -118,7 +118,7 @@ Workflow definitions use Jinja2-like `{{ expression }}` syntax for dynamic value | Boolean logic | `and`, `or`, `not` | `{{ items and status == 'ok' }}` | | Membership | `in`, `not in` | `{{ 'error' not in status }}` | | Literals | strings, numbers, booleans, lists | `{{ true }}`, `{{ [1, 2] }}` | -| Filter: `default` | `{{ val \| default('fallback') }}` | Fallback for None/empty | +| Filter: `default` | `{{ val \| default('fallback') }}` | Fallback for `None` or an empty string | | Filter: `join` | `{{ list \| join(', ') }}` | Join list elements | | Filter: `contains` | `{{ text \| contains('sub') }}` | Substring/membership check | | Filter: `map` | `{{ list \| map('attr') }}` | Extract attribute from each item | From f6628546ae6f9524f4aa487bd64c04fb26e5c1be Mon Sep 17 00:00:00 2001 From: Quratulain-bilal Date: Wed, 30 Sep 2026 22:51:54 +0500 Subject: [PATCH 7/8] fix(workflows): drop the duplicate multi-argument guard main already rejects a multi-argument filter call before the argument expression is evaluated, reporting it as an unsupported form; this branch was 53 commits behind and did not have it. The guard added in 31928a45 sat behind that check and could never fire, so its message never appeared -- which is exactly what failed in CI. Keep main's guard. Pin split(','), the filter this PR adds, against the same error, since main's coverage predates it. Assisted-by: opencode (model: mimo-v2.6-flash-free, autonomous) --- docs/reference/workflows.md | 2 +- src/specify_cli/workflows/expressions.py | 18 +------------- tests/test_workflows.py | 31 ++++++++++++------------ 3 files changed, 17 insertions(+), 34 deletions(-) diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index e5ce2672f0..099420ab02 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -651,7 +651,7 @@ Available filters: `default`, `join`, `contains`, `map`, `from_json`, `to_json`, Notes on the newer filters: -- **Types are validated, not coerced.** `upper` and `lower` accept strings only, `split` requires both a string value and a non-empty string separator, and `length` accepts lists and strings but rejects mappings. Anything else raises a `ValueError` naming the problem. Coercion is deliberately not performed: a type mismatch nearly always means the workflow is wired to the wrong variable, and a coerced result would hide that. A filter given the wrong number of arguments (`| upper('x')`, `| split` with no separator, `| split(',', 1)`) is reported as a known filter misused, which is distinct from an entirely unknown filter name; arity is checked before the argument expression is evaluated, so the extra argument is what gets named. The older filters (`join`, `map`, `contains`) are more permissive and unchanged: `join` stringifies unsupported values and `map`/`contains` return fallbacks rather than raising. +- **Types are validated, not coerced.** `upper` and `lower` accept strings only, `split` requires both a string value and a non-empty string separator, and `length` accepts lists and strings but rejects mappings. Anything else raises a `ValueError` naming the problem. Coercion is deliberately not performed: a type mismatch nearly always means the workflow is wired to the wrong variable, and a coerced result would hide that. A filter given the wrong number of arguments (`| upper('x')`, `| split` with no separator, `| split(',', 1)`) is reported as a known filter misused, which is distinct from an entirely unknown filter name: a call carrying more than one argument falls through to that same unsupported-form error rather than being evaluated as a single expression. The older filters (`join`, `map`, `contains`) are more permissive and unchanged: `join` stringifies unsupported values and `map`/`contains` return fallbacks rather than raising. - **`to_json` output is deterministic.** Object keys are sorted and non-ASCII characters are left as-is rather than escaped, so the same value always serializes to the same bytes. That buys reproducibility only — it does **not** make the result safe to pass through a shell, because [interpolation adds no quoting or escaping](#interpolation-and-shell-safety). Do not interpolate unconstrained JSON into a `run` field. - **`to_json` rejects non-finite floats.** `NaN`, `Infinity`, and `-Infinity` raise a `ValueError` naming `to_json` instead of serializing to bare tokens. None of the three is valid JSON, so emitting them would hand a standards-compliant downstream parser a string it must reject. - **`to_json` requires string mapping keys.** JSON objects have string keys, so a mapping with any other key type — `{1: "a"}`, `{True: "a"}` — raises a `ValueError` naming the key type instead. Left to `json.dumps`, an integer key would be coerced to `"1"` and collide with an existing `"1"` key, while mixed key types would fail `sort_keys` with an ordering `TypeError` reported only as "not JSON-serializable". diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index b412341726..5410da80b1 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -291,14 +291,6 @@ def _check_json_keys(value: Any) -> None: "to_json": _filter_to_json, } -# Parenthesized filters that take exactly one argument. Used to report an -# extra argument by name *before* the argument expression is evaluated: -# _evaluate_simple_expression has no comma operator, so ``| split(',', 1)`` -# would otherwise hand split an unparseable fragment, which reads back as -# "expected a string separator, got NoneType" -- the arity mistake, which is -# what the author actually got wrong, never surfaces. -_SINGLE_ARG_FILTERS = frozenset({"default", "join", "map", "contains", "split"}) - # -- Expression resolution ------------------------------------------------ @@ -682,15 +674,7 @@ def _apply_filter(value: Any, filter_expr: str, namespace: dict[str, Any]) -> An filter_match = None if filter_match: fname = filter_match.group(1) - farg_text = filter_match.group(2).strip() - if fname in _SINGLE_ARG_FILTERS: - arg_parts = _split_top_level_commas(farg_text) - if len(arg_parts) > 1: - raise ValueError( - f"{fname}: expected exactly one argument, got " - f"{len(arg_parts)}: '| {filter_expr}'" - ) - farg = _evaluate_simple_expression(farg_text, namespace) + farg = _evaluate_simple_expression(filter_match.group(2).strip(), namespace) if fname == "default": return _filter_default(value, farg) if fname == "join": diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 7d7a301eac..220e8fb7df 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -853,29 +853,28 @@ def test_filter_to_json_rejects_non_string_keys(self): == '{"1": "a", "2": "b"}' ) - def test_single_arg_filters_reject_extra_argument(self): - # Arity is checked before the argument expression is evaluated. - # split(',', 1) used to evaluate that fragment first, so the reported - # error was "expected a string separator, got NoneType" -- the arity - # mistake, which is what the author got wrong, stayed hidden behind a - # type error. + def test_split_with_extra_argument_reports_unsupported_form(self): + # split is new in this PR, so main's multi-argument guard has to cover + # it as well. Without that it evaluated the argument fragment first and + # reported "expected a string separator, got NoneType" -- the extra + # argument, which is what the author actually got wrong, never + # surfaced. main already pins default() and join() the same way; this + # pins the filter added here. import pytest from specify_cli.workflows.expressions import evaluate_expression from specify_cli.workflows.base import StepContext - ctx = StepContext(inputs={"s": "a,b,c", "n": 0}) + ctx = StepContext(inputs={"s": "a,b,c"}) with pytest.raises( - ValueError, match="split: expected exactly one argument, got 2" + ValueError, match="filter 'split' used in an unsupported form" ): evaluate_expression("{{ inputs.s | split(',', 1) }}", ctx) - with pytest.raises( - ValueError, match="default: expected exactly one argument, got 2" - ): - evaluate_expression("{{ inputs.n | default(0, 'x') }}", ctx) - # A comma inside quotes or brackets is still one argument. - assert ( - evaluate_expression("{{ inputs.s | split(',') }}", ctx) == ["a", "b", "c"] - ) + # A comma inside quotes is still a single argument. + assert evaluate_expression("{{ inputs.s | split(',') }}", ctx) == [ + "a", + "b", + "c", + ] def test_zero_arg_filters_reject_miswired_forms(self): # The strict no-argument branch is shared by from_json/upper/lower/ From 6c4469dbe4caeebd2bec4471ede0e33e5c8d4c97 Mon Sep 17 00:00:00 2001 From: Quratulain-bilal Date: Thu, 1 Oct 2026 23:38:27 +0500 Subject: [PATCH 8/8] test(workflows): correct two filter test comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The upper test said non-ASCII passes through unchanged while asserting that "café" becomes "CAFÉ": Unicode case mapping applies and nothing is transliterated or lost. The split test called split the inverse of join, which is not generally true when an element contains the separator; the assertion converts a delimiter by composing split with join. Both were the two low-severity findings on the current head. Assisted-by: opencode (model: mimo-v2.6-flash-free, autonomous) --- tests/test_workflows.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 220e8fb7df..3d16b97f3a 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -611,7 +611,7 @@ def test_filter_upper_and_lower(self): assert evaluate_expression("{{ inputs.empty | lower }}", ctx) == "" # Case-only transforms are no-ops on already-conforming input. assert evaluate_expression("{{ inputs.mixed | upper }}", ctx) == "ABC" - # Non-ASCII must pass through unchanged (no transliteration/loss). + # Non-ASCII text uses Unicode case mapping without transliteration or loss. assert evaluate_expression("{{ inputs.uni | upper }}", StepContext(inputs={"uni": "café"})) == "CAFÉ" # Filters compose left to right with the rest of the chain. assert evaluate_expression("{{ inputs.s | upper | lower }}", ctx) == "hello world" @@ -662,7 +662,7 @@ def test_filter_split(self): assert evaluate_expression("{{ inputs.nodelim | split(',') }}", ctx) == ["abc"] # Trailing empty field is preserved, so round-tripping is lossless. assert evaluate_expression("{{ inputs.trailing | split(',') }}", ctx) == ["a", "b", ""] - # split is the inverse of join. + # split composes with join to convert delimiters. assert evaluate_expression("{{ inputs.csv | split(',') | join('-') }}", ctx) == "a-b-c" def test_filter_split_rejects_non_string_inputs(self):