Skip to content

fix(workflows): refuse a filter mixed with a comparison operator instead of silently mis-binding it - #3894

Open
jawwad-ali wants to merge 1 commit into
github:mainfrom
jawwad-ali:fix/expressions-filter-operand-precedence
Open

fix(workflows): refuse a filter mixed with a comparison operator instead of silently mis-binding it#3894
jawwad-ali wants to merge 1 commit into
github:mainfrom
jawwad-ali:fix/expressions-filter-operand-precedence

Conversation

@jawwad-ali

Copy link
Copy Markdown
Contributor

Problem

In _evaluate_simple_expression the pipe is detected before the boolean and comparison operators:

pipe_idx = _find_top_level(expr, "|")
if pipe_idx != -1:
    segments = _split_top_level(expr, "|")
    value = _evaluate_simple_expression(segments[0].strip(), namespace)
    for segment in segments[1:]:
        value = _apply_filter(value, segment.strip(), namespace)
    return value

So a filter written on the right-hand operand of a comparison is applied to the comparison's boolean result, not to the operand.

Reproduction on current main (81bf741)

With inputs.count = 10 and inputs.limit missing:

{{ inputs.count > inputs.limit | default(5) }}   ->  False

count > limit is evaluated first, then default is applied to the resulting bool — which is a no-op, because a bool is never empty. The expression silently returns the comparison against the unfiltered operand. The author meant 10 > 5True.

This module already refuses the mirror case

A filter followed by a comparison is explicitly rejected rather than guessed at:

{{ inputs.missing | default('7') > '5' }}
  -> ValueError: filter 'default' used in an unsupported form

…and the test that pins it (test_filter_call_with_trailing_tokens_fails_loudly) states the reasoning:

A trailing operator/token after a filter's closing paren must not be silently discarded […] It must fall through to the "unsupported form" ValueError.

Same ambiguity, opposite handling — one raises, the other quietly returns a wrong answer. This PR applies the decision that was already made to the mirror case; it is not proposing a new precedence rule.

Fix

When the segment before the first top-level pipe contains a top-level comparison or boolean operator, report it:

ambiguous filter precedence in 'inputs.count > inputs.limit | default(5)':
  '| default(5)' would apply to the result of 'inputs.count > inputs.limit',
  not to an operand of '>'. Filter the operand in its own expression instead.

Why nothing legitimate breaks

There is no working use of a filter on a comparison result:

  • default on a bool is a no-op (a bool is never None or empty)
  • join / map / contains on a bool is an error

So every expression this now refuses was already producing either a wrong value or an error. Verified unaffected:

{{ inputs.missing | default(5) }}          -> 5
{{ inputs.items | join(", ") }}            -> 'a, b'
{{ inputs.items | map("x") | join(",") }}  -> 'a,b'
{{ inputs.items | contains("a") }}         -> True
{{ inputs.count > 5 }}                     -> True
{{ inputs.name == "x" }}                   -> True

The operator scan uses the same _find_top_level helper and the same token list as the operator parsing below it, so a > inside a quoted operand is not mistaken for one.

Verification

  • Fail-before / pass-after: test_filter_on_a_comparison_operand_is_refused fails on unpatched src and passes with the fix. tests/test_workflows.py: 21 failed → 20 failed, 838 → 839 passed.
  • The remaining 20 are identical to the clean-main baseline captured on 81bf741 (all Windows symlink-privilege).
  • uvx ruff@0.15.0 check src tests → clean

Tests sit beside the existing filter-strictness tests, and a second test pins that plain filters, chains, and bare operators are untouched.


Written with assistance from Claude Code. Bug found, reproduced, and verified by me on current main.

The pipe is detected before the boolean/comparison operators, so a filter
written on the right-hand operand was applied to the comparison's BOOLEAN
RESULT instead of to the operand:

  {{ inputs.count > inputs.limit | default(5) }}  -> False

With count=10 and limit missing, `count > limit` is evaluated first and
`default` is then applied to the resulting bool — a no-op, since a bool is
never empty — so the expression silently returns the comparison against
the *unfiltered* operand. The author meant `10 > 5` = True.

This module already refuses the mirror case rather than guessing:

  {{ inputs.missing | default('7') > '5' }}
    -> ValueError: filter 'default' used in an unsupported form

Same ambiguity, opposite handling. Refuse both the same way so an
ambiguous expression is reported instead of quietly producing the answer
the author did not ask for.

No legitimate expression is affected: applying `default` to a bool is a
no-op, and `join`/`map`/`contains` on a bool is an error, so there is no
working use of a filter on a comparison result.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jawwad-ali
jawwad-ali requested a review from mnriem as a code owner July 31, 2026 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant