diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index 35106758bf..78b57f8c8b 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -474,6 +474,12 @@ def _apply_filter(value: Any, filter_expr: str, namespace: dict[str, Any]) -> An ) +# Order matters -- multi-char operators first, so "!=" is not split as "!" + "=". +# Shared with the remediation check so a validator cannot drift from what the +# evaluator will actually split on. +_COMPARISON_OPERATORS = ("!=", "==", ">=", "<=", ">", "<", " not in ", " in ") + + def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any: """Evaluate a simple expression against the namespace. @@ -533,7 +539,7 @@ def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any: # Comparison operators (order matters — check multi-char ops first). Split at # the first top-level occurrence so an operator inside a quoted operand is # ignored. - for op in ("!=", "==", ">=", "<=", ">", "<", " not in ", " in "): + for op in _COMPARISON_OPERATORS: op_idx = _find_top_level(expr, op) if op_idx != -1: left = _evaluate_simple_expression(expr[:op_idx].strip(), namespace) @@ -879,3 +885,329 @@ def format_condition_correction(condition: Any) -> str: # double-spaced "{{ }}" that string concatenation would otherwise produce. body = "{{ " + core + " }}" if core else "{{ }}" return json.dumps(body, ensure_ascii=False) + + +def _has_unbalanced_quote(text: str) -> bool: + """True when a quote opened in *text* is never closed. + + Same left-to-right, first-quote-wins scan the rest of this module uses, so the + answer agrees with what ``_find_block_close`` and ``_strip_stray_delimiters`` + consider "inside a string". + """ + quote: str | None = None + for ch in text: + if quote is not None: + if ch == quote: + quote = None + elif ch in ("'", '"'): + quote = ch + return quote is not None + + +_BRACKET_PAIRS = {")": "(", "]": "[", "}": "{"} + +# The operators the evaluator delimits with spaces; derived so the check cannot +# drift from _COMPARISON_OPERATORS. +_WORD_OPERATORS = tuple( + op for op in (" or ", " and ") + _COMPARISON_OPERATORS if op.startswith(" ") +) + + +def _has_unbalanced_bracket(text: str) -> bool: + """True when brackets outside a quoted operand do not nest and match. + + A depth counter is not enough: it calls ``inputs.f(]`` balanced, because the + ``]`` cancels the ``(``. The evaluator then resolves that body to ``None`` and + the comparison is false, which is the inversion this module is trying to keep + out of the suggested correction. Track the opener types instead. + """ + stack: list[str] = [] + quote: str | None = None + for ch in text: + if quote is not None: + if ch == quote: + quote = None + elif ch in ("'", '"'): + quote = ch + elif ch in "([{": + stack.append(ch) + elif ch in _BRACKET_PAIRS and (not stack or stack.pop() != _BRACKET_PAIRS[ch]): + return True + return bool(stack) + + +def _has_incomplete_operand(text: str) -> bool: + """True when an operator in *text* is missing an operand on either side. + + Splits on **every** top-level occurrence rather than the first. Checking only + the first is the same defect this module exists to reject one level up: it let + ``inputs.a == inputs.b ==`` through, because the leading ``==`` has operands on + both sides and the scan stopped there. + + Reads ``_COMPARISON_OPERATORS`` from the evaluator rather than restating it, so + the check cannot drift from what ``_evaluate_simple_expression`` splits on. + """ + stripped = text.strip() + if not stripped: + return True + + # `not x` is a valid prefix form; `and x` and `or x` are not, and none of the + # three is valid alone or trailing. The keyword scans below use bare words + # because a leading operator has no space in front of it to match on. + if stripped in ("and", "or", "not") or stripped.endswith(" not"): + return True + # Word operators lose their delimiting space at the ends of a stripped core, so + # a trailing "not in" or a leading "and" needs matching without it. Derived from + # the evaluator's own table rather than restated. + for op in _WORD_OPERATORS: + if stripped.endswith(op.rstrip()) or stripped.startswith(op.lstrip()): + return True + + for op in (" or ", " and ") + _COMPARISON_OPERATORS: + if _find_top_level(stripped, op) == -1: + continue + if any(not segment.strip() for segment in _split_top_level(stripped, op)): + return True + + return _find_top_level(stripped, "|") != -1 and any( + not segment.strip() for segment in _split_top_level(stripped, "|") + ) + + +# The roots _build_namespace supplies. A reference to anything else resolves to +# None, so a correction built on one turns a truthy condition false. +_NAMESPACE_ROOTS = ("inputs", "steps", "item", "fan_in", "context") + +# Exactly what _resolve_dot_path accepts: a name, optionally one numeric index. +_PATH_SEGMENT = re.compile(r"^[\w-]+(\[\d+\])?$") + + +class _ProbeNamespace(dict): + """Namespace for the parse probe: every root exists, every leaf is absent. + + Enough for ``_evaluate_simple_expression`` to walk the grammar without needing + real inputs. Deliberately *not* resolving leaves to a sentinel value: a probe + that answers every lookup also answers ``inputs.count+1``, which is the + malformed shape the probe is meant to expose. + """ + + def __missing__(self, key: str) -> "_ProbeNamespace": # noqa: UP037 # pragma: no cover + return _ProbeNamespace() + + +def _evaluator_rejects(text: str) -> str | None: + """The evaluator's own complaint about how *text* is wired, or ``None``. + + Structural checks cannot establish that a core is parseable -- four rounds of + review found a new shape each time -- so this asks the evaluator. It reports + only the two failures ``_apply_filter`` raises about the expression itself: an + unknown filter name, and a registered filter used in an unsupported form. + + Anything else a probe run raises is about the probe's placeholder values, not + the author's text. ``steps.emit.output.stdout | from_json`` is valid against a + string output and is exercised in ``tests/test_workflows.py``; the probe hands + ``from_json`` a dict and it raises, so treating every error as a rejection + withheld a correction from a perfectly good condition. + """ + try: + _evaluate_simple_expression( + text, {root: _ProbeNamespace() for root in _NAMESPACE_ROOTS} + ) + except ValueError as exc: + message = str(exc) + # Every error _apply_filter raises about the filter *expression* quotes the + # segment back as `got '| ...'`. Its value errors instead name the type they + # received, which under a probe is the placeholder, not anything the author + # wrote -- treating those as rejections withheld corrections from valid + # conditions such as `steps.emit.output.stdout | from_json`. + if "got '| " in message: + return message.split(":", 1)[0] + except Exception: # noqa: BLE001 - probe values, not the author's text + return None + return None + + + +def _looks_numeric(text: str) -> bool: + """Mirror the evaluator's numeric literal test exactly. + + `_evaluate_simple_expression` only calls `float()` when a `.` is present and + `int()` otherwise, so `1e3` is not a number to it -- it falls through to a path + lookup and resolves to None. A bare `float()` here accepted `1e3` and the + correction turned a truthy condition false. + """ + try: + if "." in text: + float(text) + else: + int(text) + except (ValueError, TypeError): + return False + return True + + +def _is_literal(text: str) -> bool: + """Mirror the evaluator's literal tests exactly. + + The string case is the opening quote's *matching close being the final + character*, not first/last-character equality: `'a' 'b'` passes the latter but + is two literals to the evaluator, which falls through to a path lookup. + """ + if text[:1] in ("'", '"') and text.find(text[0], 1) == len(text) - 1: + return True + return text.lower() in ("true", "false", "none", "null") or _looks_numeric(text) + + +def _unresolvable_term(text: str) -> str | None: + """The first operand in *text* the evaluator cannot resolve, or ``None``. + + Walks operands the way ``_evaluate_simple_expression`` does -- filters, then + ``or``/``and``/``not``, then comparisons -- and checks each leaf. A leaf must be + a literal or a dotted path rooted in ``_NAMESPACE_ROOTS``. + + Enumerating broken shapes is what made this take several rounds: each new gate + only knew the shapes named so far. ``inputs.a === inputs.b`` split cleanly on + ``==`` and looked complete, while the evaluator read ``= inputs.b`` as a path + and resolved it to ``None``; ``bogus == 'x'`` passed for the same reason one + level up. Recursing to the leaves covers both without naming either. + """ + stripped = text.strip() + if not stripped: + return "an operand is empty" + + if _find_top_level(stripped, "|") != -1: + segments = _split_top_level(stripped, "|") + reason = _unresolvable_term(segments[0]) + if reason is not None: + return reason + # A filter argument is an ordinary operand to `_apply_filter`, which + # evaluates it with `_evaluate_simple_expression` like any other. Skipping + # it let `inputs.tags | join(bogus)` be offered as paste-ready: `bogus` is + # no namespace root, resolves to None, and the wrapped form then raises + # `join: expected a string separator, got NoneType`. Parse with the same + # pattern `_apply_filter` uses, so a form this does not recognize is left + # to the evaluator probe rather than guessed at here. + for segment in segments[1:]: + match = re.fullmatch(r"(\w+)\((.+)\)", segment.strip()) + if match is None: + continue + reason = _unresolvable_term(match.group(2)) + if reason is not None: + return reason + return None + + for op in (" or ", " and "): + idx = _find_top_level(stripped, op) + if idx != -1: + return _unresolvable_term(stripped[:idx]) or _unresolvable_term( + stripped[idx + len(op):] + ) + + if stripped.startswith("not "): + return _unresolvable_term(stripped[4:]) + + for op in _COMPARISON_OPERATORS: + idx = _find_top_level(stripped, op) + if idx != -1: + return _unresolvable_term(stripped[:idx]) or _unresolvable_term( + stripped[idx + len(op):] + ) + + if _is_literal(stripped): + return None + + # A list literal is a term the evaluator understands, and it recurses into the + # elements rather than resolving the brackets as a name. Not mirroring that + # denied the correction to `inputs.tag in ['x', 'y']` -- a condition wrapping + # repairs completely -- while reporting the list as an unresolvable name. The + # empty-segment skip matches `_evaluate_simple_expression`, which drops them so + # `[1, 2,]` is `[1, 2]` rather than `[1, 2, None]`. + if stripped.startswith("[") and stripped.endswith("]"): + inner = stripped[1:-1].strip() + if not inner: + return None + for element in _split_top_level_commas(inner): + if not element.strip(): + continue + reason = _unresolvable_term(element) + if reason is not None: + return reason + return None + + segments = _split_top_level(stripped, ".") + if not _PATH_SEGMENT.match(segments[0].strip()): + return f"{stripped!r} is not a name the evaluator can resolve" + # `item` is the only root that is not always a mapping: `StepContext.item` is + # `Any` and a fan-out assigns the item value itself, so when that value is a + # list `_resolve_dot_path` indexes it and `item[0] == 'x'` resolves. Every + # other root comes back from `_build_namespace` as a mapping, and the index + # branch returns None for those however it is written -- so the index is + # stripped for `item` alone rather than for roots in general. + root = segments[0].strip() + indexed_root = re.fullmatch(r"([\w-]+)\[\d+\]", root) + if indexed_root is not None and indexed_root.group(1) == "item": + root = indexed_root.group(1) + if root not in _NAMESPACE_ROOTS: + return ( + f"{segments[0].strip()!r} is not one of the namespace roots " + f"({', '.join(_NAMESPACE_ROOTS)})" + ) + for segment in segments[1:]: + if not _PATH_SEGMENT.match(segment.strip()): + return f"{segment.strip()!r} is not a valid path segment" + return None + + +def _wrapping_would_not_repair(core: str) -> str | None: + """Why wrapping *core* in ``{{ }}`` would not yield the expression intended. + + ``None`` when it would. Each branch names something observable about the text + itself, deliberately not the interpolator path it will take: two earlier + versions of this message asserted an internal route -- the raw-close fallback -- + and were wrong, because ``_is_single_expression`` accepts the wrapped form and + sends it down the typed fast path instead. + """ + if not core: + return "there is no expression here to wrap" + if _has_unbalanced_quote(core): + return "the quote opened in it is never closed" + if _has_unbalanced_bracket(core): + return "its brackets do not balance" + if _has_incomplete_operand(core): + return "an operator in it is missing an operand" + unresolvable = _unresolvable_term(core) + if unresolvable is not None: + return unresolvable + rejected = _evaluator_rejects(core) + if rejected is not None: + return f"the evaluator rejects it ({rejected})" + return None + + +def format_condition_remediation(condition: Any) -> str: + """The advice sentence for a condition that is never evaluated. + + ``format_condition_correction`` wraps whatever it is handed, which is right for a + formatter but wrong to advertise as paste-ready when wrapping cannot repair the + input. Measured, each of these was being offered as the fix and each **inverts** + the condition instead: + + " " -> "{{ }}" True -> False + {{ inputs.name == 'abc -> "{{ inputs.name == 'abc }}" True -> False + inputs.name == -> "{{ inputs.name == }}" True -> False + + The author is told the condition is always true, pastes the suggestion, and now + has an always-false one. Naming the fault beats handing back something that looks + authoritative and is not -- the same call already made for + ``condition_has_malformed_expression_block``, which offers no suggestion at all. + """ + core = _strip_stray_delimiters(str(condition)).strip() + reason = _wrapping_would_not_repair(core) + if reason is None: + return "Wrap the expression: " + format_condition_correction(condition) + "." + return ( + f"No correction is offered because {reason}: wrapping it as written would " + "produce a different expression from the one intended, and its result can " + "silently invert the condition rather than repair it. Complete the " + "expression, or use the literal true or false." + ) diff --git a/src/specify_cli/workflows/steps/do_while/__init__.py b/src/specify_cli/workflows/steps/do_while/__init__.py index 84921ef556..783fe44232 100644 --- a/src/specify_cli/workflows/steps/do_while/__init__.py +++ b/src/specify_cli/workflows/steps/do_while/__init__.py @@ -8,7 +8,7 @@ from specify_cli.workflows.expressions import ( condition_has_malformed_expression_block, condition_is_never_evaluated, - format_condition_correction, + format_condition_remediation, ) @@ -104,8 +104,8 @@ def validate(self, config: dict[str, Any]) -> list[str]: errors.append( f"Do-while step {config.get('id', '?')!r}: 'condition' " 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"]) + "." + "it is never evaluated as an expression and is always true. " + + format_condition_remediation(config["condition"]) ) elif condition_has_malformed_expression_block(config["condition"]): # Different fault, different advice. Here the block is *not* skipped: diff --git a/src/specify_cli/workflows/steps/if_then/__init__.py b/src/specify_cli/workflows/steps/if_then/__init__.py index cb74db7b3d..4ad2d5c9df 100644 --- a/src/specify_cli/workflows/steps/if_then/__init__.py +++ b/src/specify_cli/workflows/steps/if_then/__init__.py @@ -8,7 +8,7 @@ from specify_cli.workflows.expressions import ( condition_has_malformed_expression_block, condition_is_never_evaluated, - format_condition_correction, + format_condition_remediation, evaluate_condition, ) @@ -95,8 +95,8 @@ def validate(self, config: dict[str, Any]) -> list[str]: errors.append( f"If step {config.get('id', '?')!r}: 'condition' " 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"]) + "." + "it is never evaluated as an expression and is always true. " + + format_condition_remediation(config["condition"]) ) elif condition_has_malformed_expression_block(config["condition"]): # Different fault, different advice. Here the block is *not* skipped: diff --git a/src/specify_cli/workflows/steps/while_loop/__init__.py b/src/specify_cli/workflows/steps/while_loop/__init__.py index feda1b334d..85cd97cbb5 100644 --- a/src/specify_cli/workflows/steps/while_loop/__init__.py +++ b/src/specify_cli/workflows/steps/while_loop/__init__.py @@ -8,7 +8,7 @@ from specify_cli.workflows.expressions import ( condition_has_malformed_expression_block, condition_is_never_evaluated, - format_condition_correction, + format_condition_remediation, evaluate_condition, ) @@ -113,8 +113,8 @@ def validate(self, config: dict[str, Any]) -> list[str]: errors.append( f"While step {config.get('id', '?')!r}: 'condition' " 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"]) + "." + "it is never evaluated as an expression and is always true. " + + format_condition_remediation(config["condition"]) ) elif condition_has_malformed_expression_block(config["condition"]): # Different fault, different advice. Here the block is *not* skipped: diff --git a/tests/unit/test_condition_expression_block.py b/tests/unit/test_condition_expression_block.py index 7d9d235902..e2503f2fd8 100644 --- a/tests/unit/test_condition_expression_block.py +++ b/tests/unit/test_condition_expression_block.py @@ -9,6 +9,16 @@ condition_is_never_evaluated, evaluate_condition, format_condition_correction, + _has_unbalanced_quote, + _has_unbalanced_bracket, + _has_incomplete_operand, + _unresolvable_term, + _evaluator_rejects, + _is_literal, + _strip_stray_delimiters, + _COMPARISON_OPERATORS, + _WORD_OPERATORS, + format_condition_remediation, ) from specify_cli.workflows.steps.do_while import DoWhileStep from specify_cli.workflows.steps.if_then import IfThenStep @@ -290,3 +300,459 @@ def test_malformed_message_offers_no_paste_ready_correction(step_cls, condition) 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.") + + +# A correction is only offered when wrapping would actually repair the condition. +# These two inputs reach the same "never evaluated" branch, but wrapping them +# produces something the author must not paste, so the advice names the fault +# instead. Both were previously advertised as paste-ready (Copilot review). +UNFIXABLE_BY_WRAPPING = [ + (" ", "no expression here to wrap"), + ("{{ inputs.name == 'abc", "quote opened in it is never closed"), + ("'unterminated", "quote opened in it is never closed"), + ("inputs.name ==", "missing an operand"), + ("inputs.count >", "missing an operand"), + ("inputs.ready and", "missing an operand"), + ("inputs.x | ", "missing an operand"), + ("inputs.f(", "brackets do not balance"), +] + + +@pytest.mark.parametrize("step_cls", STEP_CLASSES) +@pytest.mark.parametrize("condition,expected", UNFIXABLE_BY_WRAPPING) +def test_no_paste_ready_correction_when_wrapping_would_not_repair( + step_cls, condition, expected +): + 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 "Wrap the expression" not in errors[0] + assert expected in errors[0] + + +def test_wrapping_whitespace_would_invert_the_condition(): + """Why the blank case gets advice instead of a suggestion. + + `{{ }}` interpolates to the empty string, so pasting it turns an always-true + condition into an always-false one -- a different defect, not a repair. + """ + ctx = StepContext(inputs={}) + assert evaluate_condition(" ", ctx) is True + assert evaluate_condition("{{ }}", ctx) is False + + +def test_wrapping_an_open_quote_inverts_the_condition(): + """Why the unbalanced-quote case gets advice instead of a suggestion. + + The raw-close fallback evaluates a truncated comparison and yields the string + "False", which evaluate_condition then reads as the `false` keyword. Pasting + the "correction" flips the condition rather than repairing it. + """ + ctx = StepContext(inputs={"name": "Bob"}) + assert evaluate_condition("{{ inputs.name == 'abc", ctx) is True + assert evaluate_condition("{{ inputs.name == 'abc }}", ctx) is False + + +@pytest.mark.parametrize( + "text,unbalanced", + [ + ("inputs.name == 'abc'", False), + ('inputs.name == "abc"', False), + ("inputs.name == 'abc", True), + ('inputs.name == "abc', True), + ("inputs.text == '\"'", False), + ("inputs.count > 100", False), + ], +) +def test_unbalanced_quote_scan(text, unbalanced): + assert _has_unbalanced_quote(text) is unbalanced + + +# The property behind the case list above, stated once so a new malformed shape +# is caught by the invariant rather than by adding another fixture row. +# Genuine expressions only. TRICKY_CONDITIONS is a quoting/escaping fixture for +# the formatter and deliberately includes prose, so it must not be reused here. +OFFERED_CORRECTION_INPUTS = [ + "inputs.count > 100", + 'inputs.name == "zzz"', + "inputs.name == 'zzz'", + "{{ inputs.count > 100", + "{{ true }} and {{ inputs.ready", + "inputs.a and inputs.b", + "inputs.name", + "not inputs.ready", + "inputs.tags | join(',')", + # The tricky-quoting cases from TRICKY_CONDITIONS that really are expressions. + # Listed rather than filtered out of that fixture, so adding prose there cannot + # silently widen what this invariant claims. + 'inputs.a == "x" and inputs.b == \'y\'', + "inputs.path == 'C:" + BACKSLASH + "tmp'", + 'inputs.path == "C:' + BACKSLASH + 'tmp"', + "inputs.a == 'x\ty'", + "inputs.a == 'x\ry'", + "inputs.ten == 'mười'", + '{{ inputs.name == "zzz"', + "}} inputs.count > 100 {{", +] + + +@pytest.mark.parametrize("condition", OFFERED_CORRECTION_INPUTS) +def test_every_offered_correction_is_a_complete_expression(condition): + """Whatever is advertised as paste-ready must pass our own validators. + + Both earlier rounds of this fix were partial because they enumerated broken + shapes -- blank, then unbalanced quote. This asserts the property instead: if + the remediation offers a correction at all, the wrapped form it hands back is + a single complete block that neither validator objects to. + """ + advice = format_condition_remediation(condition) + assert advice.startswith("Wrap the expression: ") + + suggested = yaml.safe_load( + "condition: " + advice.split("Wrap the expression: ", 1)[1].rstrip(".") + )["condition"] + assert condition_is_never_evaluated(suggested) is False + assert condition_has_malformed_expression_block(suggested) is False + + +@pytest.mark.parametrize("condition,_reason", UNFIXABLE_BY_WRAPPING) +def test_withheld_corrections_would_indeed_have_been_broken(condition, _reason): + """The other half: what is withheld really would not have survived wrapping. + + Guards against the gate growing over-eager and refusing to help with input it + could have corrected. + """ + core = _strip_stray_delimiters(condition).strip() + wrapped = "{{ " + core + " }}" + assert ( + not core + or _has_unbalanced_quote(core) + or _has_unbalanced_bracket(core) + or _has_incomplete_operand(core) + or condition_is_never_evaluated(wrapped) + or condition_has_malformed_expression_block(wrapped) + ) + + +@pytest.mark.parametrize( + "text,unbalanced", + [ + ("inputs.f(1)", False), + ("inputs.f(", True), + ("inputs.f)", True), + ("inputs.tags[0]", False), + ("inputs.text == '('", False), + ], +) +def test_unbalanced_bracket_scan(text, unbalanced): + assert _has_unbalanced_bracket(text) is unbalanced + + +def test_incomplete_operand_reads_the_evaluator_operator_list(): + """The check must not restate the operator table it is predicting.""" + for op in _COMPARISON_OPERATORS: + assert _has_incomplete_operand("inputs.a" + op) is True + assert _has_incomplete_operand("inputs.a" + op + "inputs.b") is False + + +def test_incomplete_operand_covers_every_operator_the_evaluator_splits_on(): + """Hard-coded on purpose. + + Parametrising over `_COMPARISON_OPERATORS` shrinks with the constant, so + dropping an operator from it would make that test pass vacuously -- the same + can't-fail-when-it-matters shape this module exists to reject. Listing the + operators here means removing one from the evaluator fails a test. + """ + for op in ("!=", "==", ">=", "<=", ">", "<", " not in ", " in ", " and ", " or "): + assert _has_incomplete_operand("inputs.a" + op) is True, op + assert _has_incomplete_operand("inputs.a" + op + "inputs.b") is False, op + + +# Copilot round 3: the first two gates each inspected only one position. These pin +# every-position scanning, both ends, and bracket-type matching. +MULTI_POSITION_UNFIXABLE = [ + ("inputs.a == inputs.b ==", "missing an operand"), # trailing, not the first op + ("and inputs.ready", "missing an operand"), # leading boolean operator + ("inputs.a not in", "missing an operand"), # trailing word operator + ("in inputs.tags", "missing an operand"), # leading word operator + ("inputs.f(]", "brackets do not balance"), # matched count, wrong types + ("inputs.f(]", "brackets do not balance"), + ("inputs.items | length", "the evaluator rejects it"), + ("inputs.tags | join", "used in an unsupported form"), + ('he said "hi" then left', "is not a name the evaluator can resolve"), + ("inputs.count+1", "is not a valid path segment"), + ("inputs.a === inputs.b", "is not a name the evaluator can resolve"), + ("bogus == 'x'", "is not one of the namespace roots"), + ("inputs.payload | from_json()", "the evaluator rejects it"), + # `_find_top_level` matches " and " with literal spaces, so a newline before + # the keyword is not an operator: the wrapped form evaluates False where the + # same expression with a space evaluates True. + ("inputs.x == 1\nand inputs.name == 'abc'", "is not a name the evaluator can resolve"), +] + + +@pytest.mark.parametrize("step_cls", STEP_CLASSES) +@pytest.mark.parametrize("condition,expected", MULTI_POSITION_UNFIXABLE) +def test_gates_inspect_every_position_not_just_the_first(step_cls, condition, expected): + 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 "Wrap the expression" not in errors[0] + assert expected in errors[0] + + +@pytest.mark.parametrize( + "text,unbalanced", + [ + ("inputs.f(]", True), # counts match, types do not + ("inputs.f[)", True), + ("inputs.f(}", True), + ("inputs.f([])", False), + ("inputs.f(])", True), + ("inputs.text == '(]'", False), # mismatched pair inside a quoted operand + ], +) +def test_bracket_scan_matches_types_not_just_depth(text, unbalanced): + assert _has_unbalanced_bracket(text) is unbalanced + + +def test_word_operators_are_derived_from_the_evaluator_table(): + """Guards the derivation, not the literal tuple. + + If a space-delimited operator is added to _COMPARISON_OPERATORS, the end-of-core + checks must pick it up without another edit here. + """ + assert _WORD_OPERATORS == (" or ", " and ", " not in ", " in ") + for op in _WORD_OPERATORS: + assert _has_incomplete_operand("inputs.a" + op.rstrip()) is True, op + assert _has_incomplete_operand(op.lstrip() + "inputs.a") is True, op + + +def test_the_probe_reports_what_the_evaluator_reports(): + """The parse probe must not restate the filter table. + + Four review rounds each found a shape the structural gates did not know about. + Asking the evaluator removes that class: any filter used under an unknown name + or in an unsupported form is reported by the code that will run. + """ + assert _evaluator_rejects("inputs.items | length") is not None + assert _evaluator_rejects("inputs.tags | join") is not None + assert _evaluator_rejects("inputs.tags | join(',')") is None + assert _evaluator_rejects("inputs.count > 100") is None + + +@pytest.mark.parametrize( + "text,not_a_path", + [ + ("inputs.name", False), + ("inputs.a.b.c", False), + ("inputs.tags[0]", False), + ("not inputs.ready", False), + ("true", False), + ("42", False), + ("'a literal'", False), + ("inputs.count > 100", False), # has an operator, not a bare term + ("inputs.count+1", True), # the evaluator has no arithmetic + ('he said "hi" then left', True), + # _resolve_dot_path keys on [w-]+, so a key literally named "2bad" resolves. + ("inputs.2bad", False), + ("inputs.tags[foo]", True), + ("inputs.matrix[0][1]", True), + # Round 7: an operand one level down, which the single-term gate never saw. + ("inputs.a === inputs.b", True), + ("bogus", True), + ("bogus == 'x'", True), + ("item.name == 'x'", False), + ("fan_in.results | join(',')", False), + ("context.run_id != ''", False), + ], +) +def test_operands_must_be_literals_or_known_paths(text, not_a_path): + """Recursing to the leaves replaced the single-term check. + + The old gate only looked at a core with no operator, so `inputs.a === inputs.b` + and `bogus == 'x'` walked past it. This asserts the reachable leaf instead. + """ + assert (_unresolvable_term(text) is not None) is not_a_path + + +@pytest.mark.parametrize( + "condition", + [ + # Valid against a string output and exercised in tests/test_workflows.py. + # The probe hands from_json a dict, so treating every probe error as a + # rejection withheld a correction from a good condition. + "steps.emit.output.stdout | from_json", + # The filter argument is resolved from the namespace too. + "inputs.tags | join(inputs.separator)", + ], +) +def test_probe_value_errors_are_not_treated_as_rejections(condition): + assert _evaluator_rejects(condition) is None + assert format_condition_remediation(condition).startswith("Wrap the expression: ") + + +@pytest.mark.parametrize( + "condition", + ["inputs.items | length", "inputs.tags | join"], +) +def test_filter_wiring_errors_are_still_rejections(condition): + """The other half: a filter named wrong or used wrong is the author's text.""" + assert _evaluator_rejects(condition) is not None + assert "Wrap the expression" not in format_condition_remediation(condition) + + +@pytest.mark.parametrize( + "condition,literal", + [ + ("42", True), + ("3.14", True), + ("-7", True), + # `1e3` has no "." so the evaluator calls int() on it, which fails; it then + # falls through to a path lookup. float() alone accepted it here. + ("1e3", False), + ("'one'", True), + ('"one"', True), + # Two literals, not one: the evaluator requires the opening quote's match to + # be the final character, which first/last-character equality does not. + ("'a' 'b'", False), + ("'a' == 'b'", False), + ("true", True), + ("inputs.name", False), + ], +) +def test_literal_test_mirrors_the_evaluator(condition, literal): + assert _is_literal(condition) is literal + + +@pytest.mark.parametrize( + "condition", + [ + # `_build_namespace` hands back mappings, so an indexed root always resolves + # to None however the index is written. + "inputs[0]", + "steps[1]", + "1e3", + "'a' 'b'", + ], +) +def test_shapes_the_evaluator_resolves_to_none_get_no_correction(condition): + advice = format_condition_remediation(condition) + assert "Wrap the expression" not in advice + + +# The two shapes below were each offered or withheld for the wrong reason. Both are +# checked against what the evaluator actually does with the wrapped form, not against +# a restatement of the check, so a check that drifts from the evaluator fails here. +CORRECTION_OFFERED = "Wrap the expression" + + +def _wrapped_evaluates(condition: str) -> bool: + ctx = StepContext( + inputs={ + "tag": "x", + "tags": ["a", "b"], + "count": 3, + "fallback": ", ", + "blob": '{"k": 1}', + } + ) + try: + evaluate_condition("{{ " + condition + " }}", ctx) + except Exception: + return False + return True + + +@pytest.mark.parametrize( + "condition", + [ + "inputs.tag in ['x', 'y']", + "inputs.tag not in ['x']", + "inputs.tag in [inputs.other, 'z']", + # `_evaluate_simple_expression` drops empty segments, so a trailing comma is + # `[1, 2]` rather than `[1, 2, None]`, and an empty list is a list. + "inputs.count in [1, 2,]", + "inputs.count in []", + ], +) +def test_list_literal_operands_keep_the_correction(condition): + """A list literal is a term, not a name. + + Resolving the brackets as a path reported `"['x', 'y']" is not a name the + evaluator can resolve` and withheld the correction from a condition that + wrapping repairs completely. + """ + assert CORRECTION_OFFERED in format_condition_remediation(condition) + assert _wrapped_evaluates(condition) + + +@pytest.mark.parametrize( + "condition", + ["inputs.tags | join(bogus)", "inputs.tags | map(bogus)"], +) +def test_filter_arguments_that_make_the_wrapped_form_raise_lose_the_correction(condition): + """A filter argument is an operand like any other. + + `_apply_filter` evaluates it with `_evaluate_simple_expression`, so a name that + is no namespace root arrives as None and the filter raises on it. Skipping the + argument offered these as paste-ready. + """ + assert CORRECTION_OFFERED not in format_condition_remediation(condition) + assert not _wrapped_evaluates(condition) + + +def test_a_filter_argument_that_cannot_resolve_loses_it_even_without_raising(): + """`default` tolerates the None, so this one is policy rather than a crash. + + Withholding it is the same call already made for an unresolvable name anywhere + else -- `bogus == 'x'` evaluates fine and is withheld too -- so the argument + check does not need the wrapped form to raise before it declines. + """ + condition = "inputs.count | default(bogus)" + assert CORRECTION_OFFERED not in format_condition_remediation(condition) + assert _wrapped_evaluates(condition) + assert CORRECTION_OFFERED not in format_condition_remediation("bogus == 'x'") + + +@pytest.mark.parametrize( + "condition", + [ + "inputs.tags | join(', ')", + "inputs.tags | join(inputs.fallback)", + "inputs.tags | map('name')", + "inputs.count | default(0)", + "inputs.blob | from_json", + ], +) +def test_resolvable_filter_arguments_keep_the_correction(condition): + """The other direction: the argument check must not become a blanket refusal.""" + assert CORRECTION_OFFERED in format_condition_remediation(condition) + assert _wrapped_evaluates(condition) + + +@pytest.mark.parametrize("condition", ["item[0] == 'x'", "item[1] == 'y'"]) +def test_an_indexed_item_root_keeps_the_correction(condition): + """`item` is the only root that is not always a mapping. + + `StepContext.item` is `Any` and a fan-out assigns the item value itself, so an + item that is a list makes `item[0]` resolve. Rejecting every indexed root + withheld the correction from a condition that evaluates. + """ + ctx = StepContext(inputs={"a": 1}, item=["x", "y"]) + assert CORRECTION_OFFERED in format_condition_remediation(condition) + assert evaluate_condition("{{ " + condition + " }}", ctx) is True + + +@pytest.mark.parametrize("condition", ["inputs[0]", "steps[1]", "fan_in[0]", "context[0]"]) +def test_indexing_an_always_mapping_root_still_loses_the_correction(condition): + """The other side of that split, so it does not widen into "any indexed root". + + `_build_namespace` hands these back as mappings, so `_resolve_dot_path` takes + the index branch, finds no list, and returns None however the index is written. + """ + ctx = StepContext(inputs={"a": 1}, item=["x", "y"]) + assert CORRECTION_OFFERED not in format_condition_remediation(condition) + assert evaluate_condition("{{ " + condition + " }}", ctx) is False