* fix(workflows): stop offering a correction that would not repair the condition
`format_condition_correction` wraps whatever it is handed — correct for a
formatter, wrong to advertise as paste-ready for two inputs it cannot repair.
Both reach the never-evaluated branch, and both were being suggested:
condition: " " -> "{{ }}"
{{ inputs.name == 'abc -> "{{ inputs.name == 'abc }}"
Measured what pasting each one does, rather than assuming:
" " is True -> "{{ }}" is False
"{{ inputs.name == 'abc" is True -> "{{ inputs.name == 'abc }}" is False
The blank core interpolates to the empty string. The open quote survives
wrapping, so the raw-close fallback evaluates a truncated comparison whose
result is the string "False", which `evaluate_condition` then reads as the
`false` keyword. In both cases the advertised correction silently inverts the
condition — a different defect, not a fix.
Add `format_condition_remediation`, which the three step validators now call in
place of hand-building the sentence. It offers the correction only when wrapping
would actually repair the input, and otherwise names the fault, matching the
call already made for `condition_has_malformed_expression_block`.
`_has_unbalanced_quote` uses the same left-to-right scan as `_find_block_close`
and `_strip_stray_delimiters`, so "inside a string" means the same thing
everywhere in this module.
I had the second case wrong at first and said the wrapped form "stays always
true" — the new test caught it, and the message and docstring now say inverted.
Verified on Python 3.11:
- tests/unit/test_condition_expression_block.py 133 passed (was 116)
- tests/unit + tests/test_workflows.py 1216 passed (was 1199), 22 failed
before and after — the pre-existing symlink tests needing Windows elevation.
Mutation-checked: removing either gate fails exactly the 9 new parametrised
cases and nothing else.
* fix(workflows): withhold the correction whenever wrapping cannot repair the core
Copilot found two more holes in the previous commit, and both were real.
1. The quote-balance gate was not sufficient. `inputs.name ==` has a non-empty,
quote-balanced core, so a correction was still advertised:
inputs.name == -> "{{ inputs.name == }}" True -> False
The missing operand resolves to None, the comparison evaluates False, and the
author again trades an always-true condition for an always-false one.
2. The message named the wrong mechanism. It said the wrapped form goes through
the raw-close fallback. Measured: `_is_single_expression("{{ inputs.name ==
'abc }}")` is True, so it takes the typed fast path instead.
Stop enumerating broken shapes. `_wrapping_would_not_repair` now reports the
first reason wrapping cannot yield the intended expression — empty core,
unclosed quote, unbalanced bracket, or an operator missing an operand — and the
advice names it instead of offering a suggestion.
`_has_incomplete_operand` reads `_COMPARISON_OPERATORS`, extracted from
`_evaluate_simple_expression`, so the check cannot drift from what the evaluator
actually splits on. The messages now describe the text itself rather than the
interpolator path it will take: asserting an internal route is what made the
previous two versions wrong.
Tests state the property rather than listing shapes:
`test_every_offered_correction_is_a_complete_expression` asserts that anything
advertised as paste-ready survives both validators, so a new malformed shape is
caught by the invariant rather than by another fixture row.
Verified on Python 3.11:
- tests/unit/test_condition_expression_block.py 182 passed (was 133)
- tests/unit + tests/test_workflows.py 1282 passed (was 1233), 22 failed
before and after — pre-existing symlink tests needing Windows elevation.
Mutation-checked, each gate against its own cases: dropping the operand gate
fails 12, the bracket gate 3, and removing an operator from
`_COMPARISON_OPERATORS` fails 1. That last one passed vacuously at first because
the test parametrised over the constant it was checking — the same can't-fail
shape this module rejects — so it is hard-coded now.
* fix(workflows): check every operator position and match bracket types
Copilot found two more, and both were right.
1. `_has_incomplete_operand` inspected only the first occurrence of each
operator, and its end-of-string check covered only trailing boolean keywords:
inputs.a == inputs.b == -> correction still offered, True -> False
and inputs.ready -> correction still offered, True -> False
That is the same defect this PR's parent commit fixed one level up — stopping
at the first match — reintroduced in the gate meant to prevent it. It now
splits on every top-level occurrence and requires every operand to be
non-empty.
A stripped core also loses the space that delimits a word operator, so
`inputs.a not in` matched nothing. `_WORD_OPERATORS` is derived from
`_COMPARISON_OPERATORS` and matched against both ends without it.
2. `_has_unbalanced_bracket` counted depth, so mismatched types cancelled:
inputs.f(] -> correction still offered, True -> False
It tracks opener types on a stack and rejects a non-matching closer.
The docstring Copilot flagged at line 950 is unchanged on purpose: it does not
attribute the inversion to the raw-close fallback, it records that two earlier
versions did and were wrong because `_is_single_expression` accepts the wrapped
form. That thread is marked outdated and refers to the text before `6944920`.
Verified on Python 3.11:
- tests/unit/test_condition_expression_block.py 207 passed (was 182)
- tests/unit + tests/test_workflows.py 1307 passed (was 1282), 22 failed
before and after — pre-existing symlink tests needing Windows elevation.
Mutation-checked: depth-only brackets fails 9, first-occurrence-only fails 3,
dropping the end-of-core word scan fails 16.
* fix(workflows): reject an unregistered filter and prose before suggesting a wrap
Copilot's remaining point was the strongest one on this PR: `reason is None` only
excluded four structural shapes, and structural shapes cannot establish that
wrapping produces a working expression. Two inputs proved it:
inputs.items | length -> offered; wrapped form raises
ValueError("unknown filter 'length'")
he said "hi"\nthen left -> offered; wrapped form resolves to None,
True -> False
The first replaces an always-true condition with a crash, the second inverts it.
Two checks close the gap, both reading the evaluator rather than guessing:
- `_unregistered_filter` walks the top-level `|` segments and reports the first
name missing from `_REGISTERED_FILTERS`, the same tuple `_apply_filter` raises
on.
- `_reads_as_prose` reports a core that is several bare terms with no operator
and no filter joining them. Quoted spans and bracketed groups are skipped, so
`inputs.f('a b')` and `inputs.name == 'two words'` are unaffected, and a `not `
prefix is allowed.
`he said "hi"\nthen left` was in `OFFERED_CORRECTION_INPUTS` only because that
fixture was built as `TRICKY_CONDITIONS + [...]`. TRICKY_CONDITIONS exists to
exercise the formatter's quoting and deliberately contains prose, so reusing it
asserted the wrong thing. The list is explicit now, and the tricky-quoting entries
that really are expressions are carried over by hand — adding prose to that
fixture can no longer widen what this invariant claims.
`inputs.tags | length > 0` was also mine, and `length` is not a registered
filter; it is `join(',')` now.
Verified on Python 3.11:
- tests/unit/test_condition_expression_block.py 212 passed (was 207)
- tests/unit + tests/test_workflows.py 1312 passed (was 1307), 22 failed
before and after — pre-existing symlink tests needing Windows elevation.
Mutation-checked: dropping either new gate fails 3 cases and nothing else.
* fix(workflows): ask the evaluator whether the core parses, instead of guessing
Copilot found two more shapes the structural gates did not know about:
inputs.tags | join -> offered; `join` is registered, but with no argument
`_apply_filter` raises ValueError
inputs.count+1 -> offered; the evaluator has no arithmetic, reads it as
a key named "count+1", and the wrapped form resolves
to None, turning a truthy condition false
That is the fifth shape in four rounds, which is the argument against enumerating
shapes at all. Replace the two structural checks with two that read the evaluator:
- `_evaluator_rejects` runs the core through `_evaluate_simple_expression` against
a probe namespace and returns its own error. Any filter under an unknown name or
in an unsupported form is now reported by the code that will actually run, so
`_unregistered_filter` — which restated the filter table — is gone.
- `_is_not_a_bare_path` covers what a probe cannot: a single-term core is resolved
as a path lookup, so every dotted segment must be an identifier. `count+1` is
not, and neither is prose, so `_reads_as_prose` is gone too.
The probe namespace resolves roots but not leaves, deliberately. A namespace that
answers every lookup also answers `inputs.count+1`, hiding the shape the probe
exists to expose.
Net effect is two helpers fewer and no restatement of the evaluator's tables.
Verified on Python 3.11:
- tests/unit/test_condition_expression_block.py 230 passed (was 212)
- tests/unit + tests/test_workflows.py 1330 passed (was 1312), 22 failed
before and after — pre-existing symlink tests needing Windows elevation.
Mutation-checked: dropping either check fails 6 cases and nothing else.
* fix(workflows): stop the probe rejecting valid expressions, and match the path grammar
Copilot found a false positive in the probe, which is worse than the false
negatives the earlier rounds fixed: it withheld a correction from a condition
that was already correct.
steps.emit.output.stdout | from_json -> refused
inputs.tags | join(inputs.separator) -> refused
Both are valid; the first is exercised in tests/test_workflows.py. The probe
hands `from_json` a dict and it raises, so treating every probe error as a
rejection blamed the author for the placeholder's type. `_evaluator_rejects` now
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.
Everything else a probe run raises is about probe values.
`_TERM_SUFFIX` also accepted any bracket contents and repeated indexes, while
`_resolve_dot_path` matches `^([\w-]+)\[(\d+)\]$` -- one numeric index. So
`inputs.tags[foo]` and `inputs.matrix[0][1]` passed as paths, resolved to None,
and were offered a correction that turns a truthy condition false. `_PATH_SEGMENT`
is that grammar now. It also replaces `str.isidentifier`, which was wrong in the
other direction: the resolver allows a hyphen and a leading digit in a key name.
Lint: this branch had added 3 ruff errors (2x SIM102, PIE810/UP037 on new code)
and left a top-level class without its blank lines. `ruff check` on this file is
back to the 5 pre-existing errors on `main`, all in code this PR does not touch.
On the E305 comment specifically: `ruff rule E305` reports "Selection `E305` has
no effect because preview is not enabled", and `ruff check --select E305` on this
file passes, so the repository's CI does not report it. The blank lines were still
wrong and are fixed.
Verified on Python 3.11:
- tests/unit/test_condition_expression_block.py 236 passed (was 230)
- tests/unit + tests/test_workflows.py 1336 passed (was 1330), 22 failed
before and after -- pre-existing symlink tests needing Windows elevation.
Mutation-checked: treating every probe error as a rejection fails 2, loosening
the path grammar fails 2.
* fix(workflows): validate operands recursively, and keep probe-value errors out
Copilot found three more, and the first explains why this took so many rounds:
every gate so far only inspected the shape it was written for.
inputs.a === inputs.b -> offered; splits cleanly on `==`, and the evaluator
reads `= inputs.b` as a path, resolving to None
bogus == 'x' -> offered; unknown root, same result
inputs.payload | from_json() -> offered; raises at run time
`_unresolvable_term` replaces `_is_not_a_bare_path` and walks operands the way
`_evaluate_simple_expression` does -- filters, `or`/`and`/`not`, comparisons --
down to the leaves. A leaf must be a literal or a dotted path rooted in
`_NAMESPACE_ROOTS`, the roots `_build_namespace` actually supplies. Both shapes
above fall out of that without either being named.
`_evaluator_rejects` now keeps only the errors `_apply_filter` raises about the
filter *expression*. Those quote the segment back as `got '| ...'`; its value
errors name the type they received, which under a probe is the placeholder. The
previous prefix list missed `from_json()` (a wiring error) and, when widened by
filter name, wrongly rejected `steps.emit.output.stdout | from_json` (a value
error) -- the regression the round before had just fixed.
One case fell out that no review raised: `_find_top_level` matches " and " with
literal spaces, so a newline before the keyword is not an operator.
`inputs.x == 1\nand inputs.name == 'abc'` evaluates False wrapped, where the same
expression with a space evaluates True. It was in the offered fixture; it is a
refusal case now.
Verified on Python 3.11:
- tests/unit/test_condition_expression_block.py 253 passed (was 236)
- tests/unit + tests/test_workflows.py 1353 passed (was 1336), 22 failed
before and after -- pre-existing symlink tests needing Windows elevation.
- ruff check on this file is back to the 5 errors already on main.
Mutation-checked: dropping the recursion fails 15, dropping the namespace-root
check fails 5, treating every probe error as a rejection fails 2.
* fix(workflows): mirror the evaluator's literal and root tests exactly
Three more from Copilot, all cases where my check approximated the evaluator
instead of matching it:
1e3 -> offered; no "." so the evaluator calls int(), which fails, and
it falls through to a path lookup. float() alone accepted it.
'a' 'b' -> offered; the evaluator requires the opening quote's match to be
the final character, which first/last-character equality is not.
inputs[0] -> offered; `_build_namespace` hands back mappings, so an indexed
root resolves to None however the index is written.
All three are truthy before wrapping and False after, which is the inversion this
change exists to prevent.
`_looks_numeric` and `_is_literal` now use the evaluator's own tests rather than a
looser stand-in, and the root segment is matched without stripping an index off it
first.
Not fixed, and worth being explicit about: `inputs.tags | join(5)` is still
offered. `join` always raises for a non-string separator, but that is a *type*
rule, and `_evaluator_rejects` deliberately ignores value errors because under a
probe they usually describe the placeholder rather than the author's text. The two
cannot be told apart from the message alone -- `join: expected a string separator,
got int` and `join: ..., got NoneType` differ only in a type name the probe may
have supplied. Catching it means encoding each filter's argument types in the
validator, which is the reimplementation this PR has been backing away from.
Verified on Python 3.11:
- tests/unit/test_condition_expression_block.py 267 passed (was 253)
- tests/unit + tests/test_workflows.py 1367 passed (was 1353), 22 failed
before and after -- pre-existing symlink tests needing Windows elevation.
- ruff check on this file is back to the 5 errors already on main.
Mutation-checked: restoring the bare float() fails 2, restoring the
first/last-character quote test fails 3.
* fix(workflows): mirror list literals and filter arguments in the operand check
Two shapes the leaf check did not mirror, each wrong in the opposite
direction.
A list literal is a term the evaluator understands -- it recurses into
the elements rather than resolving the brackets as a name. Resolving
them as a path reported `"['x', 'y']" is not a name the evaluator can
resolve` and withheld the correction from `inputs.tag in ['x', 'y']`,
a condition wrapping repairs completely.
A filter argument is an ordinary operand to `_apply_filter`, which
evaluates it with `_evaluate_simple_expression` like any other.
Skipping it offered `inputs.tags | join(bogus)` as paste-ready:
`bogus` is no namespace root, arrives as None, and the wrapped form
raises `join: expected a string separator, got NoneType`. Parsed with
the same pattern `_apply_filter` uses, so a form this does not
recognize is left to the evaluator probe rather than guessed at.
Every case is asserted against what the evaluator does with the
wrapped form, not against a restatement of the check.
* fix(workflows): let an indexed `item` root keep the correction
`item` is the only namespace 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. Rejecting every indexed root withheld the
correction from a condition that evaluates.
The other roots come back from `_build_namespace` as mappings, so the
index branch finds no list and returns None however the index is
written. The strip is therefore for `item` alone, and the paired test
pins that it does not widen into "any indexed root".
This narrows the root check added earlier in this branch, which was
written as though every root were a mapping.
Nguyen Thanh Dat
2dddaa54f4