From 5b682b2cb3a75dc693e9bdb1e33a169365b340f6 Mon Sep 17 00:00:00 2001 From: Noor ul ain Date: Thu, 2 Jul 2026 02:05:50 +0500 Subject: [PATCH] fix: interpolate multi-expression templates instead of returning None (#3208) (#3228) * fix: interpolate multi-expression templates instead of returning None (#3208) `evaluate_expression` returned None for templates containing two or more `{{ }}` blocks with no surrounding literal text, e.g. `"{{ context.run_id }} {{ inputs.issue }}"`. The single-expression fast path used `_EXPR_PATTERN.fullmatch()`, but `fullmatch` defeats the pattern's non-greedy `(.+?)` body: for two adjacent expressions it still matches, capturing everything between the first `{{` and the last `}}` (`"context.run_id }} {{ inputs.issue"`) as the body. That garbage failed dot-path resolution and returned None directly, bypassing the `sub()` interpolation path that would have resolved each expression. Downstream this surfaced as the literal string "None" reaching commands. Guard the fast path on `stripped.count("{{") == 1` so only genuine single-expression templates take the typed return; multi-expression templates fall through to `sub()` and interpolate correctly. Add regression tests for two expressions separated by a space and for adjacent expressions with no separator. Co-Authored-By: Claude Opus 4.8 (1M context) * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix(expressions): use match-span guard so single expressions with literal {{ keep their type The previous `stripped.count("{{") == 1` guard misclassified a genuine single expression whose string argument contains a literal `{{` (e.g. `{{ inputs.text | contains('{{') }}`) as multi-expression, routing it through `sub()` interpolation and coercing the typed (bool/int/list) return value to a string -- breaking the type-preservation the docstring promises (Copilot review on #3228). Anchor a single match at the start and require it to consume the whole stripped string instead. The non-greedy body stops at the first `}}`, so a two-block template fails the span check (falls through to interpolation, fixing #3208) while a lone expression -- including one with a `{{` inside a string literal -- matches to the end and keeps its typed value. Add a regression test for the literal-brace single-expression case. Co-Authored-By: Claude Opus 4.8 (1M context) * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix(expressions): detect single expression with quote-aware scan The match-span guard using the non-greedy _EXPR_PATTERN stopped at the first `}}`, so a lone expression whose string argument contains a literal `}}` (e.g. `{{ inputs.text | contains('}}') }}`) was misclassified as multi-expression and mis-parsed by the interpolation path, raising ValueError and turning CI red (Copilot review on #3228). Replace the span check with `_is_single_expression`, which scans the `{{ ... }}` body for a block-closing `}}` outside string literals (mirrors the quote handling already in `_split_top_level_commas`). A genuine two-block template closes early and falls through to interpolation (fixing #3208); a lone expression with a literal `{{` or `}}` inside a string argument keeps its typed return value. Co-Authored-By: Claude Opus 4.8 (1M context) * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/specify_cli/workflows/expressions.py | 56 ++++++++++++++++++++++-- tests/test_workflows.py | 34 ++++++++++++++ 2 files changed, 86 insertions(+), 4 deletions(-) diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index 6257930a5..cc63be523 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -146,6 +146,43 @@ def _build_namespace(context: Any) -> dict[str, Any]: return ns +def _is_single_expression(stripped: str) -> bool: + """True when *stripped* is exactly one top-level ``{{ ... }}`` block. + + Scans the block body for a ``}}`` that would close it early, ignoring any + braces inside string literals. This keeps a lone expression whose string + argument contains a literal ``{{`` or ``}}`` (e.g. + ``{{ inputs.text | contains('}}') }}``) on the typed fast path, while + ``{{ a }} {{ b }}`` and ``{{ a }}{{ b }}`` are correctly seen as + multi-expression. Mirrors the quote handling in + ``_split_top_level_commas``. + + A regex span check cannot decide this: the pattern's non-greedy body stops + at the first ``}}``, so a literal ``}}`` inside a string argument would be + mistaken for the closing delimiter (issue #3208, follow-up review). + """ + if not (stripped.startswith("{{") and stripped.endswith("}}")): + return False + inner = stripped[2:-2] + if not inner.strip(): + return False + quote: str | None = None + i = 0 + n = len(inner) + while i < n: + ch = inner[i] + if quote is not None: + if ch == quote: + quote = None + elif ch in ("'", '"'): + quote = ch + elif ch == "}" and i + 1 < n and inner[i + 1] == "}": + # A ``}}`` outside quotes closes the first block early. + return False + i += 1 + return True + + def _split_top_level_commas(text: str) -> list[str]: """Split *text* on commas that are not inside quotes or nested brackets. @@ -419,10 +456,21 @@ def evaluate_expression(template: str, context: Any) -> Any: namespace = _build_namespace(context) - # Single expression: return typed value - match = _EXPR_PATTERN.fullmatch(template.strip()) - if match: - return _evaluate_simple_expression(match.group(1).strip(), namespace) + # Single expression: return typed value (preserving type). + # + # The fast path must fire only when the whole template is one ``{{ ... }}`` + # block. Neither ``fullmatch`` nor a match-span check on ``_EXPR_PATTERN`` + # can decide this reliably: the non-greedy body stops at the first ``}}``, + # so ``fullmatch`` over-expands ``"{{ a }} {{ b }}"`` to garbage (returning + # ``None`` and bypassing interpolation, issue #3208), while a span check + # trips over a literal ``}}`` inside a string argument such as + # ``{{ inputs.text | contains('}}') }}`` and mis-routes it to interpolation + # (coercing its typed return to ``str``). ``_is_single_expression`` scans + # for a block-closing ``}}`` outside string literals, so both cases resolve + # correctly. + stripped = template.strip() + if _is_single_expression(stripped): + return _evaluate_simple_expression(stripped[2:-2].strip(), namespace) # Multi-expression: string interpolation def _replacer(m: re.Match[str]) -> str: diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 3f19d8eb0..2fdbf887b 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -226,6 +226,40 @@ class TestExpressions: result = evaluate_expression("Feature: {{ inputs.name }} done", ctx) assert result == "Feature: login done" + def test_multi_expression_no_surrounding_text(self): + """Two expressions with no surrounding literal text must interpolate each, + not collapse to None via the fullmatch fast path (#3208).""" + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"issue": "23"}, run_id="47c5eb4b") + result = evaluate_expression( + "{{ context.run_id }} {{ inputs.issue }}", ctx + ) + assert result == "47c5eb4b 23" + + def test_multi_expression_adjacent_no_separator(self): + """Back-to-back expressions with no separator still interpolate (#3208).""" + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"a": "foo", "b": "bar"}) + result = evaluate_expression("{{ inputs.a }}{{ inputs.b }}", ctx) + assert result == "foobar" + + def test_single_expression_with_literal_braces_preserves_type(self): + """A lone expression whose string argument contains a literal ``{{`` or ``}}`` + must still take the typed fast path and return a bool, not a string + (the fix for #3208 must not coerce it to ``\"True\"``).""" + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"text": "uses {{ jinja }} syntax"}) + assert evaluate_expression("{{ inputs.text | contains('{{') }}", ctx) is True + + ctx = StepContext(inputs={"text": "uses }} syntax"}) + assert evaluate_expression("{{ inputs.text | contains('}}') }}", ctx) is True + def test_comparison_equals(self): from specify_cli.workflows.expressions import evaluate_expression from specify_cli.workflows.base import StepContext