* fix(workflows): reject a condition that has no {{ }} block
`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.
* 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 <noreply@anthropic.com>
* 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 <noreply@anthropic.com>
* 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.
* 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.
* fix(workflows): scan every expression block, not just the first
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.
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Nguyen Thanh Dat
145e5e6889