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)