From 01ee17a687bcc181e9c41f8f6c4292c4f18c7451 Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Mon, 24 Aug 2026 10:37:12 +0700 Subject: [PATCH] fix(workflows): reject a condition that is spliced into text, not evaluated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `evaluate_expression` takes its typed fast path only when the whole string is exactly one `{{ ... }}` block. Anything else goes to `_interpolate_expressions`, which substitutes each block into the surrounding text and returns a *string*; `evaluate_condition` then coerces that with `bool()`. So a condition whose braces do not cover the whole expression is always true: {{ inputs.ready }} and {{ inputs.count > 100 }} -> "False and False" -> True not {{ inputs.ready }} -> "not False" -> True {{ inputs.count }} > 100 -> "0 > 100" -> True Each reads as a real comparison, each validates clean today, and each takes `then` on every run — a `while`/`do-while` written that way spins to `max_iterations`. The three validators already told authors the condition "is not a single complete '{{ }}' block", and nothing checked that property: `condition_is_never_evaluated` asks only whether *some* `{{` exists and closes. `condition_is_interpolated_to_text` derives the answer from `_is_single_expression` — the same predicate the fast path uses — rather than restating it, so the check cannot drift from the behaviour it predicts. It yields to both existing faults, which keep their own message and advice, and it offers no paste-ready correction: there is no single right rewrite of `{{ a }} and {{ b }}`, because only the author knows the grouping meant. --- src/specify_cli/workflows/expressions.py | 36 ++++++++++ .../workflows/steps/do_while/__init__.py | 16 +++++ .../workflows/steps/if_then/__init__.py | 16 +++++ .../workflows/steps/while_loop/__init__.py | 16 +++++ tests/unit/test_condition_expression_block.py | 71 +++++++++++++++++++ 5 files changed, 155 insertions(+) diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index 78b57f8c8b..980d9f749a 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -785,6 +785,42 @@ def condition_is_never_evaluated(condition: Any) -> bool: return _first_unclosable_block(stripped) == "verbatim" +def condition_is_interpolated_to_text(condition: Any) -> bool: + """True when *condition* holds ``{{ }}`` blocks but is spliced into text, not evaluated. + + ``evaluate_expression`` takes its typed fast path only when the whole string is + exactly one ``{{ ... }}`` block (``_is_single_expression``). Anything else — two + blocks, or one block with any text around it — goes to ``_interpolate_expressions``, + which substitutes each block into the surrounding string and returns a *string*. + ``evaluate_condition`` then coerces that with ``bool()``, so the result is true for + every rendering except ``""``, ``"true"`` and ``"false"``:: + + {{ inputs.ready }} and {{ inputs.count > 100 }} -> "False and False" -> True + not {{ inputs.ready }} -> "not False" -> True + {{ inputs.count }} > 100 -> "0 > 100" -> True + + Each of those reads as a real expression and is always true, which is the same + silent-truthiness fault ``condition_is_never_evaluated`` reports one layer out: there + the braces are missing, here they are present but do not cover the whole condition. + The operators belong *inside* one block, and the validators already tell authors the + condition must be "a single complete '{{ }}' block" -- this is the check behind that + sentence. + + Deliberately derived from ``_is_single_expression`` rather than restated, so this + cannot drift from the fast path it is predicting. + """ + if not isinstance(condition, str): + return False + stripped = condition.strip() + if not stripped or "{{" not in stripped: + return False + # Leave both of the faults that already have their own message and advice: a block + # the substituter cannot close is not an interpolation problem. + if condition_is_never_evaluated(condition) or condition_has_malformed_expression_block(condition): + return False + return not _is_single_expression(stripped) + + def condition_has_malformed_expression_block(condition: Any) -> bool: """True when *condition* holds a ``{{`` block the quote-aware scan cannot close, but which ``_interpolate_expressions`` still evaluates through its raw-close diff --git a/src/specify_cli/workflows/steps/do_while/__init__.py b/src/specify_cli/workflows/steps/do_while/__init__.py index 783fe44232..09c5763a5e 100644 --- a/src/specify_cli/workflows/steps/do_while/__init__.py +++ b/src/specify_cli/workflows/steps/do_while/__init__.py @@ -7,6 +7,7 @@ from specify_cli.workflows.base import StepBase, StepContext, StepResult, StepStatus from specify_cli.workflows.expressions import ( condition_has_malformed_expression_block, + condition_is_interpolated_to_text, condition_is_never_evaluated, format_condition_remediation, ) @@ -121,6 +122,21 @@ def validate(self, config: dict[str, Any]) -> list[str]: "truncated expression instead of the one written. Balance the " "delimiters and quotes." ) + elif condition_is_interpolated_to_text(config["condition"]): + # Third fault, third message. The braces are here and they close, but they + # do not cover the whole condition, so evaluate_expression takes its text + # path rather than the typed one: each block is substituted into the + # surrounding string and the result is coerced by bool(). Two blocks joined + # by `and` render "False and False", which is true. No paste-ready + # correction is offered: there is no single right rewrite, because only the + # author knows which grouping the operators were meant to have. + errors.append( + f"Do-while step {config.get('id', '?')!r}: 'condition' " + f"{config['condition']!r} holds more than one '{{{{ }}}}' block, or " + "text around one, so it is substituted into a string and coerced by " + "bool() instead of being evaluated. Put the whole expression inside a " + "single '{{ }}' block." + ) 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 4ad2d5c9df..0573785d90 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_has_malformed_expression_block, + condition_is_interpolated_to_text, condition_is_never_evaluated, format_condition_remediation, evaluate_condition, @@ -112,6 +113,21 @@ def validate(self, config: dict[str, Any]) -> list[str]: "truncated expression instead of the one written. Balance the " "delimiters and quotes." ) + elif condition_is_interpolated_to_text(config["condition"]): + # Third fault, third message. The braces are here and they close, but they + # do not cover the whole condition, so evaluate_expression takes its text + # path rather than the typed one: each block is substituted into the + # surrounding string and the result is coerced by bool(). Two blocks joined + # by `and` render "False and False", which is true. No paste-ready + # correction is offered: there is no single right rewrite, because only the + # author knows which grouping the operators were meant to have. + errors.append( + f"If step {config.get('id', '?')!r}: 'condition' " + f"{config['condition']!r} holds more than one '{{{{ }}}}' block, or " + "text around one, so it is substituted into a string and coerced by " + "bool() instead of being evaluated. Put the whole expression inside a " + "single '{{ }}' block." + ) 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 85cd97cbb5..8238917320 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_has_malformed_expression_block, + condition_is_interpolated_to_text, condition_is_never_evaluated, format_condition_remediation, evaluate_condition, @@ -130,6 +131,21 @@ def validate(self, config: dict[str, Any]) -> list[str]: "truncated expression instead of the one written. Balance the " "delimiters and quotes." ) + elif condition_is_interpolated_to_text(config["condition"]): + # Third fault, third message. The braces are here and they close, but they + # do not cover the whole condition, so evaluate_expression takes its text + # path rather than the typed one: each block is substituted into the + # surrounding string and the result is coerced by bool(). Two blocks joined + # by `and` render "False and False", which is true. No paste-ready + # correction is offered: there is no single right rewrite, because only the + # author knows which grouping the operators were meant to have. + errors.append( + f"While step {config.get('id', '?')!r}: 'condition' " + f"{config['condition']!r} holds more than one '{{{{ }}}}' block, or " + "text around one, so it is substituted into a string and coerced by " + "bool() instead of being evaluated. Put the whole expression inside a " + "single '{{ }}' block." + ) 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 index e2503f2fd8..0739a2bc29 100644 --- a/tests/unit/test_condition_expression_block.py +++ b/tests/unit/test_condition_expression_block.py @@ -6,8 +6,10 @@ from specify_cli.workflows.base import StepContext from specify_cli.workflows.expressions import ( condition_has_malformed_expression_block, + condition_is_interpolated_to_text, condition_is_never_evaluated, evaluate_condition, + evaluate_expression, format_condition_correction, _has_unbalanced_quote, _has_unbalanced_bracket, @@ -141,6 +143,75 @@ def test_a_malformed_block_can_raise_rather_than_be_true(): evaluate_condition("{{ inputs.missing | default('oops }}", ctx) +# A third fault. The braces are present and they close, but they do not cover the +# whole condition, so `evaluate_expression` leaves its typed fast path: each block is +# substituted into the surrounding text and the result is a *string*, which +# `evaluate_condition` then coerces. Every one of these reads as a real expression and +# is always true. The validators already told authors the condition must be "a single +# complete '{{ }}' block" -- nothing checked it. +INTERPOLATED_TO_TEXT = [ + "{{ inputs.ready }} and {{ inputs.count > 100 }}", # two blocks joined by an operator + "{{ inputs.ready }} or {{ inputs.ready }}", + "not {{ inputs.ready }}", # operator outside the block + "{{ inputs.count }} > 100", # comparison outside the block + "ready: {{ inputs.ready }}", # prose around one block + "{{ inputs.ready }}x", # a single trailing character +] + + +@pytest.mark.parametrize("condition", INTERPOLATED_TO_TEXT) +def test_a_condition_spliced_into_text_is_silently_true_and_is_flagged(condition): + # Ground truth first: the interpolated form really is a string, and really is true + # for a set of inputs where the expression the author wrote would be false. + ctx = StepContext(inputs={"ready": False, "count": 0}) + rendered = evaluate_expression(condition, ctx) + assert isinstance(rendered, str) + assert evaluate_condition(condition, ctx) is True + + assert condition_is_interpolated_to_text(condition) is True + + +@pytest.mark.parametrize("condition", INTERPOLATED_TO_TEXT) +@pytest.mark.parametrize("step_cls", STEP_CLASSES) +def test_every_condition_step_rejects_a_spliced_condition(step_cls, condition): + config = {"id": "s1", "condition": condition, "then": [], "steps": []} + errors = [e for e in step_cls().validate(config) if "'condition'" in e] + + assert len(errors) == 1 + assert "single '{{ }}' block" in errors[0] + # No paste-ready correction: there is no single right rewrite of `{{ a }} and {{ b }}`. + assert "Wrap the expression" not in errors[0] + + +VALID_SINGLE_BLOCKS = [ + "{{ inputs.ready }}", + "{{ inputs.ready and inputs.count > 100 }}", + "{{ not inputs.ready }}", + "{{ inputs.tags | join(', ') == 'a, b' }}", + # A '}}' inside a quoted argument does not end the block, so this is still one + # expression and must stay on the fast path. + "{{ inputs.text | contains('}}') }}", + # `evaluate_expression` strips before testing the fast path, so surrounding + # whitespace is not "text around the block" and must stay accepted. + " {{ inputs.ready }} ", +] + + +@pytest.mark.parametrize("condition", VALID_SINGLE_BLOCKS) +@pytest.mark.parametrize("step_cls", STEP_CLASSES) +def test_a_single_complete_block_is_still_accepted(step_cls, condition): + """The narrowing must not widen: one block, however complex, is the supported form.""" + assert condition_is_interpolated_to_text(condition) is False + config = {"id": "s1", "condition": condition, "then": [], "steps": []} + assert [e for e in step_cls().validate(config) if "'condition'" in e] == [] + + +@pytest.mark.parametrize("condition", NEVER_EVALUATED + MALFORMED_BLOCKS) +def test_the_older_two_faults_keep_their_own_message(condition): + """The new check yields to both, so each fault keeps the advice written for it.""" + assert condition_is_interpolated_to_text(condition) is False + + @pytest.mark.parametrize("condition", NEVER_EVALUATED + MALFORMED_BLOCKS) def test_the_two_faults_are_mutually_exclusive(condition): assert condition_is_never_evaluated(condition) != condition_has_malformed_expression_block(condition)