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/6] 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/6] 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/6] 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 From aa9434a74a0e08ffc2419d49cd025b34de9eb155 Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Wed, 19 Aug 2026 20:08:25 +0700 Subject: [PATCH 4/6] fix(workflows): flag a whitespace condition, and stop the correction nesting a block Two review findings, both reproduced against the code before changing it. **1. Non-empty whitespace was excluded, and it should not have been.** The docstring claimed a whitespace condition "coerces to False, which is a definite answer". That is true only of the empty string. Measured: evaluate_condition("") -> False evaluate_condition(" ") -> True evaluate_condition("\t\n ") -> True `evaluate_condition` strips only while testing the true/false keywords, then falls through to `bool()` on the raw string -- and `test_condition_whitespace_only_string_stays_truthy` pins that on purpose. So `condition: " "` is exactly the silent always-true this helper exists to catch, and it was sailing through. Fixed at validation time rather than in the evaluator, because that runtime behaviour is deliberate. The empty string stays excluded: it really does coerce to False. **2. The correction only removed edge delimiters, so it could nest one.** "prefix {{ inputs.ready" -> "{{ prefix {{ inputs.ready }}" The suggestion carried an unclosed inner block, and because its *outer* block was complete, `condition_is_never_evaluated` waved the corrected form straight back through. Same for a trailing `}}`. `_strip_stray_delimiters` now removes every delimiter, and is quote-aware for the reason the rest of this module is: `inputs.x == '}}'` holds a delimiter as data, and a blanket `re.sub` would eat it and change what the condition compares. `_find_top_level` could not be reused -- it counts `{`/`}` as bracket depth, so it never reports a `{{` as a token at all. "prefix {{ inputs.ready" -> "{{ prefix inputs.ready }}" "inputs.ready }} suffix" -> "{{ inputs.ready suffix }}" "{{ inputs.x == '}}'" -> "{{ inputs.x == '}}' }}" (data kept) '{{ inputs.name == "a b"' -> '{{ inputs.name == "a b" }}' (spacing kept) Whitespace collapses only where a delimiter was removed; inside a quoted operand it is untouched. Tests: the two fixtures that asserted whitespace was valid are corrected, and five cases added for interior delimiters, quoted delimiters and quoted spacing. 87 pass in tests/unit/test_condition_expression_block.py. tests/test_workflows.py is 20 failed / 903 passed both with and without this change -- all twenty are symlink tests that need Windows Developer Mode, and the counts are identical with the diff stashed. --- src/specify_cli/workflows/expressions.py | 66 +++++++++++++++++-- tests/unit/test_condition_expression_block.py | 56 +++++++++++++++- 2 files changed, 114 insertions(+), 8 deletions(-) diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index 80f6d91e8f..8711eb033e 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -720,13 +720,23 @@ def condition_is_never_evaluated(condition: Any) -> bool: 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. + The empty string is excluded — it coerces to ``False``, which is a definite + answer rather than a silent always-true. Non-empty whitespace is *not* + excluded: ``bool(" ")`` is true, and ``evaluate_condition`` strips only + while testing the ``true``/``false`` keywords before falling through to + ``bool()`` on the raw string. That runtime behaviour is pinned deliberately + by ``test_condition_whitespace_only_string_stays_truthy``, so the authoring + mistake has to be caught here instead: ``condition: " "`` always takes + ``then``. """ if not isinstance(condition, str): return False + if condition == "": + return False stripped = condition.strip() - if not stripped or stripped.lower() in ("true", "false"): + if not stripped: + return True + if stripped.lower() in ("true", "false"): return False open_at = stripped.find("{{") if open_at == -1: @@ -742,6 +752,47 @@ def condition_is_never_evaluated(condition: Any) -> bool: return _find_block_close(stripped, open_at) == -1 +def _strip_stray_delimiters(text: str) -> str: + """Remove every ``{{``/``}}`` that lies outside a quoted operand. + + Quote-aware for the same reason the rest of this module is: ``inputs.x == '}}'`` + holds a delimiter as *data*, and a blanket ``re.sub`` would eat it and change + what the corrected condition compares against. Whitespace orphaned by a removed + delimiter collapses to one separator so the suggestion still reads as an + expression; whitespace inside a quoted operand is never touched. + + ``_find_top_level`` cannot serve here: it counts ``{`` and ``}`` as bracket + depth, so it never reports a ``{{`` as a top-level token at all. + """ + out: list[str] = [] + quote: str | None = None + i = 0 + n = len(text) + while i < n: + ch = text[i] + if quote is not None: + out.append(ch) + if ch == quote: + quote = None + i += 1 + continue + if ch in ("'", '"'): + quote = ch + out.append(ch) + i += 1 + continue + if text.startswith("{{", i) or text.startswith("}}", i): + i += 2 + while i < n and text[i].isspace(): + i += 1 + while out and out[-1].isspace(): + out.pop() + out.append(" ") + continue + out.append(ch) + i += 1 + return "".join(out) + def format_condition_correction(condition: Any) -> str: """Render *condition* wrapped in ``{{ }}`` as a quoted, paste-ready YAML scalar. @@ -761,8 +812,11 @@ def format_condition_correction(condition: Any) -> str: A stray delimiter is dropped rather than nested: ``{{ inputs.count > 100`` corrects to ``"{{ inputs.count > 100 }}"``, not to a doubled ``{{ {{ ... }} }}``. + Every stray delimiter goes, not only the ones sitting at the edges. Trimming + just the edges left ``prefix {{ inputs.ready`` reading + ``"{{ prefix {{ inputs.ready }}"`` -- an unclosed inner block, and one whose + complete *outer* block then carried the correction straight back through + ``condition_is_never_evaluated`` as if it were valid. """ - core = str(condition).strip() - core = re.sub(r"^\s*(\{\{|\}\})\s*", "", core) - core = re.sub(r"\s*(\{\{|\}\})\s*$", "", core).strip() + core = _strip_stray_delimiters(str(condition)).strip() 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 ba54381485..7089ea58d7 100644 --- a/tests/unit/test_condition_expression_block.py +++ b/tests/unit/test_condition_expression_block.py @@ -42,7 +42,7 @@ def test_validator_rejects_condition_without_expression_block(step_cls): @pytest.mark.parametrize("step_cls", STEP_CLASSES) @pytest.mark.parametrize( "condition", - ["{{ inputs.count > 100 }}", "true", "false", "TRUE", True, False, "", " "], + ["{{ 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.""" @@ -59,7 +59,11 @@ def test_validator_accepts_evaluated_and_literal_conditions(step_cls, condition) ("true", False), ("False", False), ("", False), - (" ", False), + # `bool(" ")` is true and evaluate_condition strips only around the + # true/false keywords, so whitespace is a silent always-true, not a + # definite False. Only "" coerces to False. + (" ", True), + ("\t\n ", True), (True, False), (["a"], False), (3, False), @@ -167,3 +171,51 @@ def test_correction_keeps_non_ascii_readable(): corrected = format_condition_correction("inputs.ten == 'mười'") assert "mười" in corrected assert chr(92) + "u" not in corrected + + +def test_whitespace_condition_is_flagged_but_the_empty_string_is_not(): + """Whitespace is the silent always-true this validator exists to catch. + + ``test_condition_whitespace_only_string_stays_truthy`` pins the runtime + behaviour deliberately, so the mistake can only be caught at validation time. + """ + assert evaluate_condition(" ", StepContext()) is True + assert condition_is_never_evaluated(" ") is True + + assert evaluate_condition("", StepContext()) is False + assert condition_is_never_evaluated("") is False + + +@pytest.mark.parametrize( + "condition", + [ + "prefix {{ inputs.ready", + "inputs.ready }} suffix", + "{{ inputs.a }} and {{ inputs.b", + ], +) +def test_correction_removes_an_interior_delimiter_too(condition): + """Trimming only the edges left the correction carrying an inner block. + + ``prefix {{ inputs.ready`` corrected to ``"{{ prefix {{ inputs.ready }}"``, + whose complete outer block then walked back past this very validator. + """ + corrected = format_condition_correction(condition) + inner = yaml.safe_load("condition: " + corrected)["condition"] + assert inner.count("{{") == 1 and inner.count("}}") == 1 + assert inner.startswith("{{ ") and inner.endswith(" }}") + + +def test_correction_keeps_a_delimiter_that_is_quoted_data(): + """``'}}'`` is an operand, not a block, so the stripper must not eat it.""" + corrected = format_condition_correction("{{ inputs.x == '}}'") + inner = yaml.safe_load("condition: " + corrected)["condition"] + assert inner == "{{ inputs.x == '}}' }}" + assert condition_is_never_evaluated(inner) is False + + +def test_correction_preserves_spacing_inside_a_quoted_operand(): + """Whitespace is collapsed only where a delimiter was removed.""" + corrected = format_condition_correction('{{ inputs.name == "a b"') + inner = yaml.safe_load("condition: " + corrected)["condition"] + assert inner == '{{ inputs.name == "a b" }}' From 3e28b32aa949967578c9aa04f366dff903bd3f27 Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Thu, 20 Aug 2026 07:49:57 +0700 Subject: [PATCH 5/6] fix(workflows): separate a malformed block from one that is never evaluated Third review finding, and like the first two it reproduces. `condition_is_never_evaluated` returned True for any `{{` the quote-aware scan could not close -- but `_interpolate_expressions` does not treat those alike. Its own comment spells out two sub-cases, and only one is "never evaluated": * no raw `}}` in the tail -> the text is emitted verbatim, so bool() makes it true. Genuinely uninterpolated. * a raw `}}` further along -> that is used as the close and the truncated body *is* evaluated. Measured: {{ inputs.count > 100 -> True (never evaluated) }} inputs.count > 100 {{ -> True (never evaluated) {{ inputs.x == '}}' -> True (raw-close path) {{ inputs.missing | default('oops }} -> raises ValueError That last one made the old message wrong on both halves: it is evaluated, and it does not end up true -- it ends the run in `_apply_filter`. Adds `condition_has_malformed_expression_block` and gives it its own branch in the three validators, because the two faults need opposite advice: one says "you forgot the braces", the other says "your delimiters or quotes do not balance". The two predicates are mutually exclusive, pinned by a test over every fixture. The malformed branch deliberately offers **no** paste-ready correction. The fault is unbalanced quoting, so the quote-aware stripper cannot tell operand from delimiter -- for `{{ inputs.missing | default('oops }}` it emits `"{{ inputs.missing | default('oops }} }}"`, which is not a fix. This is the same "avoid offering an automatic correction for malformed-block cases" the reviewer raised earlier; it applies exactly here. Also renders a blank correction as `"{{ }}"` rather than the double-spaced `"{{ }}"` that concatenation produced for a whitespace-only condition. 106 pass in tests/unit/test_condition_expression_block.py. Across tests/test_workflows.py + tests/unit the run is 22 failed / 1189 passed, and 22 failed / 1170 passed with this diff stashed -- identical failures, all Windows symlink cases, none touching conditions or expressions. --- src/specify_cli/workflows/expressions.py | 56 ++++++++++++--- .../workflows/steps/do_while/__init__.py | 15 ++++ .../workflows/steps/if_then/__init__.py | 15 ++++ .../workflows/steps/while_loop/__init__.py | 15 ++++ tests/unit/test_condition_expression_block.py | 72 ++++++++++++++++++- 5 files changed, 161 insertions(+), 12 deletions(-) diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index 8711eb033e..78eeffc2f9 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -742,14 +742,49 @@ def condition_is_never_evaluated(condition: Any) -> bool: if open_at == -1: return True # 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 + # one -- but only when the substituter really does leave it alone. + # ``_interpolate_expressions`` has two sub-cases when its quote-aware scan + # fails, and they do not behave alike: with no raw ``}}`` in the tail the + # block is emitted verbatim (never evaluated, so ``bool()`` makes it true), + # while a raw ``}}`` further along is used as the close and the truncated + # body *is* evaluated. Only the first is "never evaluated"; see + # ``condition_has_malformed_expression_block`` for the second. + if _find_block_close(stripped, open_at) != -1: + return False + return stripped.find("}}", open_at + 2) == -1 + + +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 + fallback. + + This is a different fault from the one + ``condition_is_never_evaluated`` reports, and it deserves a different message. + The block is not skipped: the interpolator takes the first raw ``}}`` after the + opener and evaluates whatever it truncated, so + + {{ inputs.missing | default('oops }} + + reaches ``_apply_filter`` and raises ``ValueError`` at run time. Calling that + "never evaluated and always true" is wrong twice over -- it is evaluated, and it + does not end up true, it ends the run. + + Kept separate from the never-evaluated check rather than folded in, because the + two need opposite advice: one says "you forgot the braces", this one says "your + delimiters or quotes do not balance". + """ + if not isinstance(condition, str): + return False + stripped = condition.strip() + if not stripped or stripped.lower() in ("true", "false"): + return False + open_at = stripped.find("{{") + if open_at == -1: + return False + if _find_block_close(stripped, open_at) != -1: + return False + return stripped.find("}}", open_at + 2) != -1 def _strip_stray_delimiters(text: str) -> str: @@ -819,4 +854,7 @@ def format_condition_correction(condition: Any) -> str: ``condition_is_never_evaluated`` as if it were valid. """ core = _strip_stray_delimiters(str(condition)).strip() - return json.dumps("{{ " + core + " }}", ensure_ascii=False) + # A blank core has nothing to wrap; render the empty block rather than the + # double-spaced "{{ }}" that string concatenation would otherwise produce. + body = "{{ " + core + " }}" if core else "{{ }}" + return json.dumps(body, ensure_ascii=False) diff --git a/src/specify_cli/workflows/steps/do_while/__init__.py b/src/specify_cli/workflows/steps/do_while/__init__.py index 4b5428abe8..b6d01210fc 100644 --- a/src/specify_cli/workflows/steps/do_while/__init__.py +++ b/src/specify_cli/workflows/steps/do_while/__init__.py @@ -6,6 +6,7 @@ from specify_cli.workflows.base import StepBase, StepContext, StepResult, StepStatus from specify_cli.workflows.expressions import ( + condition_has_malformed_expression_block, condition_is_never_evaluated, format_condition_correction, ) @@ -106,6 +107,20 @@ def validate(self, config: dict[str, Any]) -> list[str]: "never evaluated and is always true. Wrap the expression: " + format_condition_correction(config["condition"]) + "." ) + elif condition_has_malformed_expression_block(config["condition"]): + # Different fault, different advice. Here the block is *not* skipped: + # _interpolate_expressions cannot close it with its quote-aware scan, so it + # falls back to the first raw close and evaluates whatever that truncated. + # `{{ inputs.missing | default('oops }}` reaches the filter parser and raises + # ValueError at run time, so reporting it as "always true" would be wrong + # twice over: it is evaluated, and it does not end up true. + errors.append( + f"Do-while step {config.get('id', '?')!r}: 'condition' " + f"{config['condition']!r} opens a '{{{{' the interpolator cannot " + "close, so it falls back to the first raw '}}' and evaluates a " + "truncated expression instead of the one written. Balance the " + "delimiters and quotes." + ) 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 2c154fa956..6c6878e0bf 100644 --- a/src/specify_cli/workflows/steps/if_then/__init__.py +++ b/src/specify_cli/workflows/steps/if_then/__init__.py @@ -6,6 +6,7 @@ from specify_cli.workflows.base import StepBase, StepContext, StepResult, StepStatus from specify_cli.workflows.expressions import ( + condition_has_malformed_expression_block, condition_is_never_evaluated, format_condition_correction, evaluate_condition, @@ -97,6 +98,20 @@ def validate(self, config: dict[str, Any]) -> list[str]: "never evaluated and is always true. Wrap the expression: " + format_condition_correction(config["condition"]) + "." ) + elif condition_has_malformed_expression_block(config["condition"]): + # Different fault, different advice. Here the block is *not* skipped: + # _interpolate_expressions cannot close it with its quote-aware scan, so it + # falls back to the first raw close and evaluates whatever that truncated. + # `{{ inputs.missing | default('oops }}` reaches the filter parser and raises + # ValueError at run time, so reporting it as "always true" would be wrong + # twice over: it is evaluated, and it does not end up true. + errors.append( + f"If step {config.get('id', '?')!r}: 'condition' " + f"{config['condition']!r} opens a '{{{{' the interpolator cannot " + "close, so it falls back to the first raw '}}' and evaluates a " + "truncated expression instead of the one written. Balance the " + "delimiters and quotes." + ) 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 9319b99c3d..99ebbf4344 100644 --- a/src/specify_cli/workflows/steps/while_loop/__init__.py +++ b/src/specify_cli/workflows/steps/while_loop/__init__.py @@ -6,6 +6,7 @@ from specify_cli.workflows.base import StepBase, StepContext, StepResult, StepStatus from specify_cli.workflows.expressions import ( + condition_has_malformed_expression_block, condition_is_never_evaluated, format_condition_correction, evaluate_condition, @@ -115,6 +116,20 @@ def validate(self, config: dict[str, Any]) -> list[str]: "never evaluated and is always true. Wrap the expression: " + format_condition_correction(config["condition"]) + "." ) + elif condition_has_malformed_expression_block(config["condition"]): + # Different fault, different advice. Here the block is *not* skipped: + # _interpolate_expressions cannot close it with its quote-aware scan, so it + # falls back to the first raw close and evaluates whatever that truncated. + # `{{ inputs.missing | default('oops }}` reaches the filter parser and raises + # ValueError at run time, so reporting it as "always true" would be wrong + # twice over: it is evaluated, and it does not end up true. + errors.append( + f"While step {config.get('id', '?')!r}: 'condition' " + f"{config['condition']!r} opens a '{{{{' the interpolator cannot " + "close, so it falls back to the first raw '}}' and evaluates a " + "truncated expression instead of the one written. Balance the " + "delimiters and quotes." + ) 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 7089ea58d7..aeeb759bba 100644 --- a/tests/unit/test_condition_expression_block.py +++ b/tests/unit/test_condition_expression_block.py @@ -5,6 +5,7 @@ from specify_cli.workflows.base import StepContext from specify_cli.workflows.expressions import ( + condition_has_malformed_expression_block, condition_is_never_evaluated, evaluate_condition, format_condition_correction, @@ -86,10 +87,16 @@ 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. +] + +# A different fault, and the interpolator treats it differently: the quote-aware +# scan finds no close, but a raw '}}' exists further along, so +# _interpolate_expressions falls back to it and *evaluates* the truncated body. +# These are not "never evaluated" -- one leaves residual text that bool() makes +# true, the other reaches the filter parser and raises. +MALFORMED_BLOCKS = [ "{{ inputs.x == '}}'", + "{{ inputs.missing | default('oops }}", ] @@ -98,6 +105,30 @@ 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 + assert condition_has_malformed_expression_block(condition) is False + + +@pytest.mark.parametrize("condition", MALFORMED_BLOCKS) +def test_raw_close_fallback_is_malformed_not_never_evaluated(condition): + """The block *is* evaluated, so it must not be reported as always true.""" + assert condition_has_malformed_expression_block(condition) is True + assert condition_is_never_evaluated(condition) is False + + +def test_a_malformed_block_can_raise_rather_than_be_true(): + """The concrete case the "always true" wording got wrong. + + `default('oops` swallows the real close, the raw-close fallback hands the + filter parser a truncated argument, and the run dies instead of taking a branch. + """ + ctx = StepContext(inputs={"count": 5}) + with pytest.raises(ValueError): + evaluate_condition("{{ inputs.missing | default('oops }}", ctx) + + +@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) @pytest.mark.parametrize( @@ -219,3 +250,38 @@ def test_correction_preserves_spacing_inside_a_quoted_operand(): corrected = format_condition_correction('{{ inputs.name == "a b"') inner = yaml.safe_load("condition: " + corrected)["condition"] assert inner == '{{ inputs.name == "a b" }}' + + +@pytest.mark.parametrize("step_cls", STEP_CLASSES) +@pytest.mark.parametrize("condition", MALFORMED_BLOCKS) +def test_validator_reports_malformed_rather_than_always_true(step_cls, condition): + """The two faults need opposite advice, so they must not share a message. + + "never evaluated and is always true" is wrong here on both halves: the + interpolator does evaluate the truncated body, and the result is not + reliably true -- it can raise. + """ + 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 "never evaluated" not in errors[0] + assert "cannot close" in errors[0] + assert "truncated expression" in errors[0] + + +@pytest.mark.parametrize("step_cls", STEP_CLASSES) +@pytest.mark.parametrize("condition", MALFORMED_BLOCKS) +def test_malformed_message_offers_no_paste_ready_correction(step_cls, condition): + """Deliberately no suggestion for this class. + + The fault is unbalanced delimiters or quotes, so the quote-aware stripper + cannot tell operand from delimiter -- for `{{ inputs.missing | default('oops }}` + it produces `"{{ inputs.missing | default('oops }} }}"`, which is not a fix. + Naming the fault beats handing back something that looks authoritative and + is not. + """ + config = {"id": "s1", "condition": condition, "then": [], "steps": []} + errors = [e for e in step_cls().validate(config) if "'condition'" in e] + assert "Wrap the expression" not in errors[0] + assert errors[0].rstrip().endswith("Balance the delimiters and quotes.") From 183b633ce718a5264edd58bd3509568ddaa85a86 Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Thu, 20 Aug 2026 20:04:56 +0700 Subject: [PATCH 6/6] fix(workflows): scan every expression block, not just the first MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both condition validators stopped at the first `{{`. A condition whose first block closes was accepted regardless of what followed, so a later unterminated block escaped validation entirely — the case Copilot raised: {{ true }} and {{ inputs.ready -> both validators returned False Interpolation leaves `and {{ inputs.ready` in the result and bool() makes the condition always true, which is exactly the silent-branching defect these validators exist to catch. The same hole applied to the malformed class: {{ inputs.name }} {{ inputs.missing | default('oops }} -> raises at run time Add `_first_unclosable_block`, which walks blocks the way `_interpolate_expressions` does — continuing past each block that closes — and reports how the first unclosable one will fail: `evaluated` when a raw `}}` follows (the fallback truncates and evaluates), `verbatim` when none does. Both validators now read from it, so they cannot disagree with the substitution they predict. Two wording fixes fall out of scanning further: - The never-evaluated message said the condition "has no complete '{{ }}' block". With an earlier complete block that is false, so it now says the condition "is not a single complete '{{ }}' block". - `condition_has_malformed_expression_block`'s docstring said the truncated body raises ValueError. It does for `default('oops`, but `{{ inputs.x == '}}'` evaluates to the residual `"False'"` instead. Measured both; the docstring now says either can happen and the error message never claimed otherwise. Verified on Python 3.11: - tests/unit/test_condition_expression_block.py 116 passed (was 106) - tests/unit + tests/test_workflows.py 1199 passed (was 1189), 22 failed before and after — all pre-existing symlink tests that need Windows elevation. Mutation-checked: restoring the stop-after-first-block behaviour fails exactly the 10 new parametrised cases and nothing else. --- src/specify_cli/workflows/expressions.py | 49 +++++++++++++------ .../workflows/steps/do_while/__init__.py | 4 +- .../workflows/steps/if_then/__init__.py | 4 +- .../workflows/steps/while_loop/__init__.py | 4 +- tests/unit/test_condition_expression_block.py | 5 ++ 5 files changed, 46 insertions(+), 20 deletions(-) diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index 78eeffc2f9..35106758bf 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -249,6 +249,34 @@ def _find_block_close(text: str, start: int) -> int: return -1 +def _first_unclosable_block(text: str) -> str | None: + """How ``_interpolate_expressions`` will fail on the first block it cannot + close with the quote-aware scan, or ``None`` when every block closes. + + Returns ``"evaluated"`` when a raw ``}}`` still follows the opener -- the + interpolator falls back to it and evaluates the truncated body, which reaches + the filter parser and raises ``ValueError``. Returns ``"verbatim"`` when no + ``}}`` follows at all -- the tail is emitted unchanged, so it survives into the + result as truthy text. + + Walks blocks exactly the way ``_interpolate_expressions`` does, continuing past + each block that *does* close. Checking only the first opener let a later + unterminated block through both validators: ``{{ true }} and {{ inputs.ready`` + closes its first block, so the scan stopped and reported no fault, while + interpolation leaves ``and {{ inputs.ready`` in the result and ``bool()`` makes + the condition always true. + """ + i = 0 + while True: + start = text.find("{{", i) + if start == -1: + return None + close = _find_block_close(text, start) + if close == -1: + return "evaluated" if text.find("}}", start + 2) != -1 else "verbatim" + i = close + 2 + + def _interpolate_expressions(template: str, namespace: dict[str, Any]) -> str: """Substitute every top-level ``{{ ... }}`` block in *template*, quote-aware. @@ -738,8 +766,7 @@ def condition_is_never_evaluated(condition: Any) -> bool: return True if stripped.lower() in ("true", "false"): return False - open_at = stripped.find("{{") - if open_at == -1: + if "{{" not in stripped: return True # An opening ``{{`` the substituter cannot close is no better than a missing # one -- but only when the substituter really does leave it alone. @@ -749,9 +776,7 @@ def condition_is_never_evaluated(condition: Any) -> bool: # while a raw ``}}`` further along is used as the close and the truncated # body *is* evaluated. Only the first is "never evaluated"; see # ``condition_has_malformed_expression_block`` for the second. - if _find_block_close(stripped, open_at) != -1: - return False - return stripped.find("}}", open_at + 2) == -1 + return _first_unclosable_block(stripped) == "verbatim" def condition_has_malformed_expression_block(condition: Any) -> bool: @@ -766,9 +791,10 @@ def condition_has_malformed_expression_block(condition: Any) -> bool: {{ inputs.missing | default('oops }} - reaches ``_apply_filter`` and raises ``ValueError`` at run time. Calling that - "never evaluated and always true" is wrong twice over -- it is evaluated, and it - does not end up true, it ends the run. + reaches ``_apply_filter`` and raises ``ValueError`` at run time. The truncation does + not always raise -- ``{{ inputs.x == '}}'`` evaluates to the residual ``"False'"`` -- + but either way what runs is not what was written, so "never evaluated and always + true" is the wrong report. Kept separate from the never-evaluated check rather than folded in, because the two need opposite advice: one says "you forgot the braces", this one says "your @@ -779,12 +805,7 @@ def condition_has_malformed_expression_block(condition: Any) -> bool: stripped = condition.strip() if not stripped or stripped.lower() in ("true", "false"): return False - open_at = stripped.find("{{") - if open_at == -1: - return False - if _find_block_close(stripped, open_at) != -1: - return False - return stripped.find("}}", open_at + 2) != -1 + return _first_unclosable_block(stripped) == "evaluated" def _strip_stray_delimiters(text: str) -> str: diff --git a/src/specify_cli/workflows/steps/do_while/__init__.py b/src/specify_cli/workflows/steps/do_while/__init__.py index b6d01210fc..84921ef556 100644 --- a/src/specify_cli/workflows/steps/do_while/__init__.py +++ b/src/specify_cli/workflows/steps/do_while/__init__.py @@ -103,8 +103,8 @@ 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 complete '{{{{ }}}}' block, so it is " - "never evaluated and is always true. Wrap the expression: " + f"{config['condition']!r} is not a single complete '{{{{ }}}}' block, so " + "it is never evaluated as an expression and is always true. Wrap the expression: " + format_condition_correction(config["condition"]) + "." ) elif condition_has_malformed_expression_block(config["condition"]): diff --git a/src/specify_cli/workflows/steps/if_then/__init__.py b/src/specify_cli/workflows/steps/if_then/__init__.py index 6c6878e0bf..cb74db7b3d 100644 --- a/src/specify_cli/workflows/steps/if_then/__init__.py +++ b/src/specify_cli/workflows/steps/if_then/__init__.py @@ -94,8 +94,8 @@ 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 complete '{{{{ }}}}' block, so it is " - "never evaluated and is always true. Wrap the expression: " + f"{config['condition']!r} is not a single complete '{{{{ }}}}' block, so " + "it is never evaluated as an expression and is always true. Wrap the expression: " + format_condition_correction(config["condition"]) + "." ) elif condition_has_malformed_expression_block(config["condition"]): diff --git a/src/specify_cli/workflows/steps/while_loop/__init__.py b/src/specify_cli/workflows/steps/while_loop/__init__.py index 99ebbf4344..feda1b334d 100644 --- a/src/specify_cli/workflows/steps/while_loop/__init__.py +++ b/src/specify_cli/workflows/steps/while_loop/__init__.py @@ -112,8 +112,8 @@ 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 complete '{{{{ }}}}' block, so it is " - "never evaluated and is always true. Wrap the expression: " + f"{config['condition']!r} is not a single complete '{{{{ }}}}' block, so " + "it is never evaluated as an expression and is always true. Wrap the expression: " + format_condition_correction(config["condition"]) + "." ) elif condition_has_malformed_expression_block(config["condition"]): diff --git a/tests/unit/test_condition_expression_block.py b/tests/unit/test_condition_expression_block.py index aeeb759bba..7d9d235902 100644 --- a/tests/unit/test_condition_expression_block.py +++ b/tests/unit/test_condition_expression_block.py @@ -87,6 +87,9 @@ 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 + # A complete block does not vouch for the rest: interpolation leaves the + # second fragment verbatim, and bool() makes the whole string true. + "{{ true }} and {{ inputs.ready", ] # A different fault, and the interpolator treats it differently: the quote-aware @@ -97,6 +100,8 @@ def test_condition_is_never_evaluated(value, expected): MALFORMED_BLOCKS = [ "{{ inputs.x == '}}'", "{{ inputs.missing | default('oops }}", + # Same, but the faulty block is the second one. + "{{ inputs.name }} {{ inputs.missing | default('oops }}", ]