From 0fa9e8764c8fc5644351685cbd15a4ce70eef1df Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Tue, 18 Aug 2026 17:03:48 +0700 Subject: [PATCH 1/3] fix(workflows): reject a condition that has no {{ }} block MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `evaluate_condition` resolves its argument through `evaluate_expression`, which only substitutes `{{ ... }}` blocks. A string with no such block comes back unchanged and — unless it reads `true`/`false` — is then coerced by `bool()`. So a condition authored without the braces is never evaluated at all: evaluate_condition("inputs.count > 100", ctx) -> True evaluate_condition("{{ inputs.count > 100 }}", ctx) -> False with `inputs.count == 5` in both cases. An `if` step always takes `then`, and a `while`/`do-while` step always runs to `max_iterations` — ten agent invocations for a loop the author expected to stop. This is the same silent-truthiness authoring mistake the three step validators already reject for a list/dict/number condition, and it is easier to make: GitHub Actions accepts a bare expression in `if:`, so the brace-less form is a habit to bring here. Adds `condition_is_never_evaluated()` and wires it into the `if`, `while` and `do-while` validators, so the mistake surfaces at validation with the corrected form spelled out. Boolean literals, real bools, empty strings and any string containing `{{` stay valid — runtime behaviour is unchanged. --- src/specify_cli/workflows/expressions.py | 27 ++++++++ .../workflows/steps/do_while/__init__.py | 15 +++++ .../workflows/steps/if_then/__init__.py | 19 +++++- .../workflows/steps/while_loop/__init__.py | 19 +++++- tests/unit/test_condition_expression_block.py | 67 +++++++++++++++++++ 5 files changed, 145 insertions(+), 2 deletions(-) create mode 100644 tests/unit/test_condition_expression_block.py diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index 38a29890ae..7d5169289e 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -690,3 +690,30 @@ def evaluate_condition(condition: str, context: Any) -> bool: if lower == "true": return True return bool(result) + + +def condition_is_never_evaluated(condition: Any) -> bool: + """True when a string *condition* is silently treated as always-true text. + + ``evaluate_condition`` resolves its argument through + ``evaluate_expression``, which only substitutes ``{{ ... }}`` blocks. A + string with no such block comes back unchanged, and — unless it reads + ``true``/``false`` — is then coerced by ``bool()``. So an expression + authored without the braces, e.g. ``condition: inputs.count > 100``, is + never evaluated at all: it is a non-empty string, so the ``if`` step always + takes ``then`` and a ``while``/``do-while`` step always runs to + ``max_iterations``. + + That is the same silent-truthiness authoring mistake the step validators + already reject for a list/dict/number condition, and it is easy to write: + GitHub Actions accepts a bare expression in ``if:``. + + An empty/whitespace string is excluded — it coerces to ``False``, which is + a definite answer rather than a silent always-true. + """ + if not isinstance(condition, str): + return False + stripped = condition.strip() + if not stripped or stripped.lower() in ("true", "false"): + return False + return "{{" not in stripped diff --git a/src/specify_cli/workflows/steps/do_while/__init__.py b/src/specify_cli/workflows/steps/do_while/__init__.py index 024ced55b5..80e4013653 100644 --- a/src/specify_cli/workflows/steps/do_while/__init__.py +++ b/src/specify_cli/workflows/steps/do_while/__init__.py @@ -5,6 +5,7 @@ from typing import Any from specify_cli.workflows.base import StepBase, StepContext, StepResult, StepStatus +from specify_cli.workflows.expressions import condition_is_never_evaluated class DoWhileStep(StepBase): @@ -88,6 +89,20 @@ def validate(self, config: dict[str, Any]) -> list[str]: f"Do-while step {config.get('id', '?')!r}: 'condition' must be a " f"string or boolean, got {type(config['condition']).__name__}." ) + elif condition_is_never_evaluated(config["condition"]): + # A string condition with no ``{{ }}`` block is never evaluated: + # evaluate_expression() returns it unchanged and bool() then makes + # any non-empty text true. `condition: inputs.count > 100` reads as + # a real comparison but always takes every iteration. This is the same + # silent-truthiness mistake the list/dict branch above rejects, and + # GitHub Actions accepts a bare expression in `if:`, so it is easy + # to write by habit. + errors.append( + f"Do-while step {config.get('id', '?')!r}: 'condition' " + f"{config['condition']!r} has no '{{{{ }}}}' block, so it is never " + "evaluated and is always true. Wrap the expression: " + '"{{ ' + str(config["condition"]).strip() + ' }}".' + ) max_iter = config.get("max_iterations") if max_iter is not None: # bool is a subclass of int, so isinstance(True, int) is True and diff --git a/src/specify_cli/workflows/steps/if_then/__init__.py b/src/specify_cli/workflows/steps/if_then/__init__.py index 7189ff8150..90ec7eac7c 100644 --- a/src/specify_cli/workflows/steps/if_then/__init__.py +++ b/src/specify_cli/workflows/steps/if_then/__init__.py @@ -5,7 +5,10 @@ from typing import Any from specify_cli.workflows.base import StepBase, StepContext, StepResult, StepStatus -from specify_cli.workflows.expressions import evaluate_condition +from specify_cli.workflows.expressions import ( + condition_is_never_evaluated, + evaluate_condition, +) class IfThenStep(StepBase): @@ -79,6 +82,20 @@ def validate(self, config: dict[str, Any]) -> list[str]: f"If step {config.get('id', '?')!r}: 'condition' must be a " f"string or boolean, got {type(config['condition']).__name__}." ) + elif condition_is_never_evaluated(config["condition"]): + # A string condition with no ``{{ }}`` block is never evaluated: + # evaluate_expression() returns it unchanged and bool() then makes + # any non-empty text true. `condition: inputs.count > 100` reads as + # a real comparison but always takes ``then``. This is the same + # silent-truthiness mistake the list/dict branch above rejects, and + # GitHub Actions accepts a bare expression in `if:`, so it is easy + # to write by habit. + errors.append( + f"If step {config.get('id', '?')!r}: 'condition' " + f"{config['condition']!r} has no '{{{{ }}}}' block, so it is never " + "evaluated and is always true. Wrap the expression: " + '"{{ ' + str(config["condition"]).strip() + ' }}".' + ) if "then" not in config: errors.append( f"If step {config.get('id', '?')!r} is missing 'then' field." diff --git a/src/specify_cli/workflows/steps/while_loop/__init__.py b/src/specify_cli/workflows/steps/while_loop/__init__.py index e80b93d7f2..d19cbdb510 100644 --- a/src/specify_cli/workflows/steps/while_loop/__init__.py +++ b/src/specify_cli/workflows/steps/while_loop/__init__.py @@ -5,7 +5,10 @@ from typing import Any from specify_cli.workflows.base import StepBase, StepContext, StepResult, StepStatus -from specify_cli.workflows.expressions import evaluate_condition +from specify_cli.workflows.expressions import ( + condition_is_never_evaluated, + evaluate_condition, +) class WhileStep(StepBase): @@ -97,6 +100,20 @@ def validate(self, config: dict[str, Any]) -> list[str]: f"While step {config.get('id', '?')!r}: 'condition' must be a " f"string or boolean, got {type(config['condition']).__name__}." ) + elif condition_is_never_evaluated(config["condition"]): + # A string condition with no ``{{ }}`` block is never evaluated: + # evaluate_expression() returns it unchanged and bool() then makes + # any non-empty text true. `condition: inputs.count > 100` reads as + # a real comparison but always takes every iteration. This is the same + # silent-truthiness mistake the list/dict branch above rejects, and + # GitHub Actions accepts a bare expression in `if:`, so it is easy + # to write by habit. + errors.append( + f"While step {config.get('id', '?')!r}: 'condition' " + f"{config['condition']!r} has no '{{{{ }}}}' block, so it is never " + "evaluated and is always true. Wrap the expression: " + '"{{ ' + str(config["condition"]).strip() + ' }}".' + ) max_iter = config.get("max_iterations") if max_iter is not None: # bool is a subclass of int, so isinstance(True, int) is True and diff --git a/tests/unit/test_condition_expression_block.py b/tests/unit/test_condition_expression_block.py new file mode 100644 index 0000000000..f7c9e4bc50 --- /dev/null +++ b/tests/unit/test_condition_expression_block.py @@ -0,0 +1,67 @@ +"""A string condition with no ``{{ }}`` block is never evaluated (always true).""" + +import pytest + +from specify_cli.workflows.base import StepContext +from specify_cli.workflows.expressions import ( + condition_is_never_evaluated, + evaluate_condition, +) +from specify_cli.workflows.steps.do_while import DoWhileStep +from specify_cli.workflows.steps.if_then import IfThenStep +from specify_cli.workflows.steps.while_loop import WhileStep + +STEP_CLASSES = [IfThenStep, WhileStep, DoWhileStep] + + +@pytest.mark.parametrize( + "condition", + ["inputs.count > 100", "inputs.name == 'zzz'", "inputs.count < 3"], +) +def test_brace_less_condition_is_always_true_at_runtime(condition): + """The behaviour the validator now warns about, pinned so it cannot drift.""" + ctx = StepContext(inputs={"count": 5, "name": "abc"}) + # Same expression with braces resolves to its real (false) value... + assert evaluate_condition("{{ " + condition + " }}", ctx) is False + # ...without them it is only non-empty text, so bool() makes it true. + assert evaluate_condition(condition, ctx) is True + + +@pytest.mark.parametrize("step_cls", STEP_CLASSES) +def test_validator_rejects_condition_without_expression_block(step_cls): + config = {"id": "s1", "condition": "inputs.count > 100", "then": [], "steps": []} + errors = [e for e in step_cls().validate(config) if "never evaluated" in e] + assert len(errors) == 1 + assert "inputs.count > 100" in errors[0] + # The message hands back the corrected form. + assert '"{{ inputs.count > 100 }}"' in errors[0] + + +@pytest.mark.parametrize("step_cls", STEP_CLASSES) +@pytest.mark.parametrize( + "condition", + ["{{ inputs.count > 100 }}", "true", "false", "TRUE", True, False, "", " "], +) +def test_validator_accepts_evaluated_and_literal_conditions(step_cls, condition): + """No false positives: braces, boolean literals and bools stay valid.""" + config = {"id": "s1", "condition": condition, "then": [], "steps": []} + assert not [e for e in step_cls().validate(config) if "never evaluated" in e] + + +@pytest.mark.parametrize( + ("value", "expected"), + [ + ("inputs.count > 100", True), + ("{{ inputs.count > 100 }}", False), + ("prefix {{ inputs.a }} suffix", False), + ("true", False), + ("False", False), + ("", False), + (" ", False), + (True, False), + (["a"], False), + (3, False), + ], +) +def test_condition_is_never_evaluated(value, expected): + assert condition_is_never_evaluated(value) is expected From c0c291cbd870381a6d8c075e7447f23e5a28f5d8 Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Tue, 18 Aug 2026 20:04:03 +0700 Subject: [PATCH 2/3] fix(workflows): flag an unterminated {{ and quote the correction safely Two gaps in the condition validator, both raised in review. An opening `{{` with no `}}` after it is never substituted either: _interpolate_expressions takes its `raw_close == -1` branch and appends the tail verbatim. So `condition: "{{ inputs.count > 100"` -- and the reversed `"}} inputs.count > 100 {{"`, whose only `{{` is last -- come back unchanged and are coerced to true exactly like a brace-less string. The helper now looks for a complete block rather than an opening one. The suggested correction was interpolated into a double-quoted scalar, so a condition containing a double quote produced YAML that does not parse: `condition: "{{ inputs.name == "zzz" }}"` raises a ParserError. format_condition_correction() now picks the quoting from the content and drops a stray delimiter instead of nesting a second one, so the message stays paste-ready. All three validators share it. Tests: 30 more cases -- the incomplete forms, and a YAML round trip over conditions holding single quotes, double quotes, both, and backslashes, asserting each correction loads back exactly and is not re-flagged. Co-Authored-By: Claude Opus 5 --- src/specify_cli/workflows/expressions.py | 35 +++++++- .../workflows/steps/do_while/__init__.py | 11 ++- .../workflows/steps/if_then/__init__.py | 7 +- .../workflows/steps/while_loop/__init__.py | 7 +- tests/unit/test_condition_expression_block.py | 83 +++++++++++++++++++ 5 files changed, 132 insertions(+), 11 deletions(-) diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index 7d5169289e..e9f5ad635e 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -716,4 +716,37 @@ def condition_is_never_evaluated(condition: Any) -> bool: stripped = condition.strip() if not stripped or stripped.lower() in ("true", "false"): return False - return "{{" not in stripped + open_at = stripped.find("{{") + if open_at == -1: + return True + # An opening ``{{`` with no ``}}`` anywhere after it is never substituted + # either: ``_interpolate_expressions`` takes its ``raw_close == -1`` branch + # and appends the tail verbatim. So ``{{ inputs.count > 100`` -- and the + # reversed ``}} inputs.count > 100 {{``, whose only ``{{`` is last -- come + # back unchanged and are just as silently true as a brace-less string. + return stripped.find("}}", open_at + 2) == -1 + + +def format_condition_correction(condition: Any) -> str: + """Render *condition* wrapped in ``{{ }}`` as a quoted, paste-ready YAML scalar. + + The validators hand this back as the corrected form, so it has to survive a + round trip through a YAML parser. A plain ``"{{ ... }}"`` does not: a + condition holding a double quote (``inputs.name == "zzz"``) closes the + scalar early and the workflow file no longer loads. Quoting is therefore + chosen from the content -- double by default, single when the expression + itself contains a double quote, and double with backslash escapes when it + contains both. + + A stray delimiter is dropped rather than nested: ``{{ inputs.count > 100`` + corrects to ``"{{ inputs.count > 100 }}"``, not to a doubled ``{{ {{ ... }} }}``. + """ + core = str(condition).strip() + core = re.sub(r"^\s*(\{\{|\}\})\s*", "", core) + core = re.sub(r"\s*(\{\{|\}\})\s*$", "", core).strip() + wrapped = "{{ " + core + " }}" + if '"' not in wrapped and "\\" not in wrapped: + return '"' + wrapped + '"' + if "'" not in wrapped: + return "'" + wrapped + "'" + return '"' + wrapped.replace("\\", "\\\\").replace('"', '\\"') + '"' diff --git a/src/specify_cli/workflows/steps/do_while/__init__.py b/src/specify_cli/workflows/steps/do_while/__init__.py index 80e4013653..4b5428abe8 100644 --- a/src/specify_cli/workflows/steps/do_while/__init__.py +++ b/src/specify_cli/workflows/steps/do_while/__init__.py @@ -5,7 +5,10 @@ from typing import Any from specify_cli.workflows.base import StepBase, StepContext, StepResult, StepStatus -from specify_cli.workflows.expressions import condition_is_never_evaluated +from specify_cli.workflows.expressions import ( + condition_is_never_evaluated, + format_condition_correction, +) class DoWhileStep(StepBase): @@ -99,9 +102,9 @@ def validate(self, config: dict[str, Any]) -> list[str]: # to write by habit. errors.append( f"Do-while step {config.get('id', '?')!r}: 'condition' " - f"{config['condition']!r} has no '{{{{ }}}}' block, so it is never " - "evaluated and is always true. Wrap the expression: " - '"{{ ' + str(config["condition"]).strip() + ' }}".' + f"{config['condition']!r} has no complete '{{{{ }}}}' block, so it is " + "never evaluated and is always true. Wrap the expression: " + + format_condition_correction(config["condition"]) + "." ) max_iter = config.get("max_iterations") if max_iter is not None: diff --git a/src/specify_cli/workflows/steps/if_then/__init__.py b/src/specify_cli/workflows/steps/if_then/__init__.py index 90ec7eac7c..2c154fa956 100644 --- a/src/specify_cli/workflows/steps/if_then/__init__.py +++ b/src/specify_cli/workflows/steps/if_then/__init__.py @@ -7,6 +7,7 @@ from specify_cli.workflows.base import StepBase, StepContext, StepResult, StepStatus from specify_cli.workflows.expressions import ( condition_is_never_evaluated, + format_condition_correction, evaluate_condition, ) @@ -92,9 +93,9 @@ def validate(self, config: dict[str, Any]) -> list[str]: # to write by habit. errors.append( f"If step {config.get('id', '?')!r}: 'condition' " - f"{config['condition']!r} has no '{{{{ }}}}' block, so it is never " - "evaluated and is always true. Wrap the expression: " - '"{{ ' + str(config["condition"]).strip() + ' }}".' + f"{config['condition']!r} has no complete '{{{{ }}}}' block, so it is " + "never evaluated and is always true. Wrap the expression: " + + format_condition_correction(config["condition"]) + "." ) if "then" not in config: errors.append( diff --git a/src/specify_cli/workflows/steps/while_loop/__init__.py b/src/specify_cli/workflows/steps/while_loop/__init__.py index d19cbdb510..9319b99c3d 100644 --- a/src/specify_cli/workflows/steps/while_loop/__init__.py +++ b/src/specify_cli/workflows/steps/while_loop/__init__.py @@ -7,6 +7,7 @@ from specify_cli.workflows.base import StepBase, StepContext, StepResult, StepStatus from specify_cli.workflows.expressions import ( condition_is_never_evaluated, + format_condition_correction, evaluate_condition, ) @@ -110,9 +111,9 @@ def validate(self, config: dict[str, Any]) -> list[str]: # to write by habit. errors.append( f"While step {config.get('id', '?')!r}: 'condition' " - f"{config['condition']!r} has no '{{{{ }}}}' block, so it is never " - "evaluated and is always true. Wrap the expression: " - '"{{ ' + str(config["condition"]).strip() + ' }}".' + f"{config['condition']!r} has no complete '{{{{ }}}}' block, so it is " + "never evaluated and is always true. Wrap the expression: " + + format_condition_correction(config["condition"]) + "." ) max_iter = config.get("max_iterations") if max_iter is not None: diff --git a/tests/unit/test_condition_expression_block.py b/tests/unit/test_condition_expression_block.py index f7c9e4bc50..c93c8174c2 100644 --- a/tests/unit/test_condition_expression_block.py +++ b/tests/unit/test_condition_expression_block.py @@ -1,11 +1,13 @@ """A string condition with no ``{{ }}`` block is never evaluated (always true).""" import pytest +import yaml from specify_cli.workflows.base import StepContext from specify_cli.workflows.expressions import ( condition_is_never_evaluated, evaluate_condition, + format_condition_correction, ) from specify_cli.workflows.steps.do_while import DoWhileStep from specify_cli.workflows.steps.if_then import IfThenStep @@ -65,3 +67,84 @@ def test_validator_accepts_evaluated_and_literal_conditions(step_cls, condition) ) def test_condition_is_never_evaluated(value, expected): assert condition_is_never_evaluated(value) is expected + + +# --- An unterminated ``{{`` is the same defect, not a different one ----------- +# +# ``_interpolate_expressions`` substitutes nothing when no ``}}`` follows the +# opening ``{{`` (its ``raw_close == -1`` branch appends the tail verbatim), so +# ``{{ inputs.count > 100`` is returned unchanged and coerced to true exactly +# like a brace-less string. + +BACKSLASH = chr(92) + +NEVER_EVALUATED = [ + "inputs.count > 100", # no delimiter at all + "{{ inputs.count > 100", # opened, never closed + "}} inputs.count > 100 {{", # reversed: the only '{{' is last +] + + +@pytest.mark.parametrize("condition", NEVER_EVALUATED) +def test_incomplete_block_is_silently_true_and_is_flagged(condition): + ctx = StepContext(inputs={"count": 5, "name": "abc"}) + assert evaluate_condition(condition, ctx) is True + assert condition_is_never_evaluated(condition) is True + + +@pytest.mark.parametrize( + "condition", + [ + "{{ inputs.count > 100 }}", + "{{ inputs.a }} and {{ inputs.b }}", + "{{ inputs.text | default('}}') }}", # literal '}}' inside an argument + ], +) +def test_complete_block_is_not_flagged(condition): + assert condition_is_never_evaluated(condition) is False + + +# --- The suggested correction has to survive a YAML round trip --------------- + +TRICKY_CONDITIONS = [ + "inputs.count > 100", + 'inputs.name == "zzz"', # double quote + "inputs.name == 'zzz'", # single quote + 'inputs.a == "x" and inputs.b == \'y\'', # both + "inputs.path == 'C:" + BACKSLASH + "tmp'", # backslash + 'inputs.path == "C:' + BACKSLASH + 'tmp"', # backslash + quote + '{{ inputs.name == "zzz"', # incomplete + quote + "}} inputs.count > 100 {{", +] + + +@pytest.mark.parametrize("condition", TRICKY_CONDITIONS) +def test_correction_is_valid_yaml_and_round_trips(condition): + """A correction the author cannot paste into their workflow is no correction.""" + loaded = yaml.safe_load("condition: " + format_condition_correction(condition)) + stripped = condition.strip().lstrip("{}").rstrip("{}").strip() + assert loaded["condition"] == "{{ " + stripped + " }}" + + +@pytest.mark.parametrize("condition", TRICKY_CONDITIONS) +def test_correction_does_not_trip_the_validator_again(condition): + loaded = yaml.safe_load("condition: " + format_condition_correction(condition)) + assert condition_is_never_evaluated(loaded["condition"]) is False + + +@pytest.mark.parametrize("condition", ["{{ inputs.count > 100", "}} a > 1 {{"]) +def test_correction_replaces_a_stray_delimiter_instead_of_nesting_one(condition): + corrected = format_condition_correction(condition) + assert "{{ {{" not in corrected and "}} }}" not in corrected + assert corrected.count("{{") == 1 and corrected.count("}}") == 1 + + +@pytest.mark.parametrize("step_cls", STEP_CLASSES) +@pytest.mark.parametrize("condition", ['inputs.name == "zzz"', "{{ inputs.count > 100"]) +def test_validator_correction_is_yaml_safe(step_cls, condition): + config = {"id": "s1", "condition": condition, "then": [], "steps": []} + errors = [e for e in step_cls().validate(config) if "never evaluated" in e] + assert len(errors) == 1 + suggested = errors[0].split("Wrap the expression: ", 1)[1].rstrip(".") + loaded = yaml.safe_load("condition: " + suggested) + assert condition_is_never_evaluated(loaded["condition"]) is False From dc3c7dee2900df36afaaad6a386db57435feb850 Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Wed, 19 Aug 2026 02:09:14 +0700 Subject: [PATCH 3/3] fix(workflows): share the evaluator's quote-aware scan, and quote with json.dumps Both follow-up review points were right. The completeness check used a plain `find("}}")`, but the substituter closes a block with a quote-aware scan. So `condition: "{{ inputs.x == '}}'"` looked complete to the validator while `_interpolate_expressions` found no close, fell to its raw-close branch, evaluated a truncated body and left residual text (`False'`) -- a non-empty string, hence true. Rather than restate the quote rules a third time, the scan moves out of `_interpolate_expressions` into `_find_block_close`, which the validator now calls: the check and the substitution it predicts can no longer disagree. A `}}` that is genuinely inside a string argument still does not close early, so `{{ inputs.text | default('}}') }}` and `{{ inputs.x == '}}' }}` stay accepted. The correction's quoting enumerated the characters it escaped, and the enumeration was short: a condition loaded from a YAML literal block can carry a newline, which a double-quoted scalar folds, so the corrected form did not round-trip. `json.dumps` decides it instead -- every JSON string is a valid YAML double-quoted scalar and it escapes quotes, backslashes, newlines and the other control characters. `ensure_ascii=False` keeps a non-ASCII operand readable rather than expanding it into numeric escapes. Tests: 70 -> 83. The quoted-delimiter condition joins the incomplete-block set, and the round-trip set gains multiline, newline-with-quote, tab, carriage return and non-ASCII operands. All four new cases fail on the previous commit. Co-Authored-By: Claude Opus 5 --- src/specify_cli/workflows/expressions.py | 74 +++++++++++-------- tests/unit/test_condition_expression_block.py | 19 +++++ 2 files changed, 64 insertions(+), 29 deletions(-) diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index e9f5ad635e..80f6d91e8f 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -224,6 +224,31 @@ def _is_single_expression(stripped: str) -> bool: return True +def _find_block_close(text: str, start: int) -> int: + """Index of the ``}}`` closing the block opened by the ``{{`` at *start*, or -1. + + Quote-aware, so a literal ``}}`` inside a string argument + (``{{ inputs.text | default('}}') }}``) does not close the block early -- + the same rule ``_is_single_expression`` applies. Shared with + ``condition_is_never_evaluated`` so the validator cannot disagree with the + substitution it is predicting. + """ + quote: str | None = None + i = start + 2 + n = len(text) + while i < n: + ch = text[i] + if quote is not None: + if ch == quote: + quote = None + elif ch in ("'", '"'): + quote = ch + elif ch == "}" and i + 1 < n and text[i + 1] == "}": + return i + i += 1 + return -1 + + def _interpolate_expressions(template: str, namespace: dict[str, Any]) -> str: """Substitute every top-level ``{{ ... }}`` block in *template*, quote-aware. @@ -249,20 +274,7 @@ def _interpolate_expressions(template: str, namespace: dict[str, Any]) -> str: break out.append(template[i:start]) # Scan for the block-closing ``}}`` that is outside any string literal. - j = start + 2 - quote: str | None = None - close = -1 - while j < n: - ch = template[j] - if quote is not None: - if ch == quote: - quote = None - elif ch in ("'", '"'): - quote = ch - elif ch == "}" and j + 1 < n and template[j + 1] == "}": - close = j - break - j += 1 + close = _find_block_close(template, start) if close == -1: # No quote-aware close. Two sub-cases, both kept identical to the old # regex so a malformed template is never silently hidden: @@ -719,12 +731,15 @@ def condition_is_never_evaluated(condition: Any) -> bool: open_at = stripped.find("{{") if open_at == -1: return True - # An opening ``{{`` with no ``}}`` anywhere after it is never substituted - # either: ``_interpolate_expressions`` takes its ``raw_close == -1`` branch - # and appends the tail verbatim. So ``{{ inputs.count > 100`` -- and the - # reversed ``}} inputs.count > 100 {{``, whose only ``{{`` is last -- come - # back unchanged and are just as silently true as a brace-less string. - return stripped.find("}}", open_at + 2) == -1 + # An opening ``{{`` the substituter cannot close is no better than a missing + # one. ``_interpolate_expressions`` closes a block with the same quote-aware + # scan used here, so ``{{ inputs.count > 100`` -- and the reversed + # ``}} inputs.count > 100 {{``, whose only ``{{`` is last -- come back + # verbatim, while ``{{ inputs.x == '}}'`` falls to the raw-close branch and + # leaves residual text (``False'``). All three are non-empty strings that + # ``bool()`` then makes true. A plain ``find("}}")`` would miss the third, + # and would also have to re-derive quote handling this module already owns. + return _find_block_close(stripped, open_at) == -1 def format_condition_correction(condition: Any) -> str: @@ -734,9 +749,15 @@ def format_condition_correction(condition: Any) -> str: round trip through a YAML parser. A plain ``"{{ ... }}"`` does not: a condition holding a double quote (``inputs.name == "zzz"``) closes the scalar early and the workflow file no longer loads. Quoting is therefore - chosen from the content -- double by default, single when the expression - itself contains a double quote, and double with backslash escapes when it - contains both. + chosen from the content. That enumeration was incomplete: a condition loaded + from a YAML literal block can carry a newline, which a double-quoted scalar + folds, so the correction did not round-trip. + + ``json.dumps`` decides it instead. Every JSON string is a valid YAML + double-quoted scalar, and it escapes the quotes, backslashes, newlines and + other control characters that hand-rolled quoting has to enumerate. + ``ensure_ascii=False`` keeps non-ASCII operands readable rather than + expanding them into numeric escapes. A stray delimiter is dropped rather than nested: ``{{ inputs.count > 100`` corrects to ``"{{ inputs.count > 100 }}"``, not to a doubled ``{{ {{ ... }} }}``. @@ -744,9 +765,4 @@ def format_condition_correction(condition: Any) -> str: core = str(condition).strip() core = re.sub(r"^\s*(\{\{|\}\})\s*", "", core) core = re.sub(r"\s*(\{\{|\}\})\s*$", "", core).strip() - wrapped = "{{ " + core + " }}" - if '"' not in wrapped and "\\" not in wrapped: - return '"' + wrapped + '"' - if "'" not in wrapped: - return "'" + wrapped + "'" - return '"' + wrapped.replace("\\", "\\\\").replace('"', '\\"') + '"' + return json.dumps("{{ " + core + " }}", ensure_ascii=False) diff --git a/tests/unit/test_condition_expression_block.py b/tests/unit/test_condition_expression_block.py index c93c8174c2..ba54381485 100644 --- a/tests/unit/test_condition_expression_block.py +++ b/tests/unit/test_condition_expression_block.py @@ -82,6 +82,10 @@ def test_condition_is_never_evaluated(value, expected): "inputs.count > 100", # no delimiter at all "{{ inputs.count > 100", # opened, never closed "}} inputs.count > 100 {{", # reversed: the only '{{' is last + # The only '}}' sits inside a string operand, so the quote-aware scan finds + # no close. The raw-close fallback then evaluates a truncated body and + # leaves residual text ("False'"), which bool() makes true just the same. + "{{ inputs.x == '}}'", ] @@ -98,6 +102,7 @@ def test_incomplete_block_is_silently_true_and_is_flagged(condition): "{{ inputs.count > 100 }}", "{{ inputs.a }} and {{ inputs.b }}", "{{ inputs.text | default('}}') }}", # literal '}}' inside an argument + "{{ inputs.x == '}}' }}", # quoted '}}' then the real close ], ) def test_complete_block_is_not_flagged(condition): @@ -115,6 +120,13 @@ def test_complete_block_is_not_flagged(condition): 'inputs.path == "C:' + BACKSLASH + 'tmp"', # backslash + quote '{{ inputs.name == "zzz"', # incomplete + quote "}} inputs.count > 100 {{", + # A YAML literal block hands the loader a real newline; a folded scalar + # would lose it, so the correction has to escape rather than embed it. + "inputs.x == 1\nand inputs.name == 'abc'", + 'he said "hi"\nthen left', # newline + quote + "inputs.a == 'x\ty'", # tab + "inputs.a == 'x\ry'", # carriage return + "inputs.ten == 'mười'", # non-ASCII operand ] @@ -148,3 +160,10 @@ def test_validator_correction_is_yaml_safe(step_cls, condition): suggested = errors[0].split("Wrap the expression: ", 1)[1].rstrip(".") loaded = yaml.safe_load("condition: " + suggested) assert condition_is_never_evaluated(loaded["condition"]) is False + + +def test_correction_keeps_non_ascii_readable(): + """ensure_ascii=False: an operand should not turn into numeric escapes.""" + corrected = format_condition_correction("inputs.ten == 'mười'") + assert "mười" in corrected + assert chr(92) + "u" not in corrected