fix(workflows): stop offering a condition correction that inverts it - #4230
fix(workflows): stop offering a condition correction that inverts it#4230ntdatt812 wants to merge 8 commits into
Conversation
…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.
There was a problem hiding this comment.
Pull request overview
Centralizes condition-remediation advice to avoid suggesting known condition inversions.
Changes:
- Adds guarded remediation for blank and unbalanced-quote conditions.
- Updates all conditional step validators.
- Adds regression tests for unsafe corrections.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/workflows/expressions.py |
Adds remediation logic and quote scanning. |
src/specify_cli/workflows/steps/if_then/__init__.py |
Uses centralized remediation. |
src/specify_cli/workflows/steps/while_loop/__init__.py |
Uses centralized remediation. |
src/specify_cli/workflows/steps/do_while/__init__.py |
Uses centralized remediation. |
tests/unit/test_condition_expression_block.py |
Covers correction inversions and quote detection. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (2)
src/specify_cli/workflows/expressions.py:930
- The quote-balance check is not enough to establish that wrapping repairs the condition. For example,
inputs.name ==has a nonempty, quote-balanced core, so this still advertises"{{ inputs.name == }}"; the original is truthy text, but the suggested form resolves the missing RHS toNoneand evaluates the comparison asFalse. The paste-ready correction therefore still silently inverts malformed inputs. Validate the core for incomplete operators/brackets before offering a correction, or withhold the correction when its syntax cannot be established.
if _has_unbalanced_quote(core):
src/specify_cli/workflows/expressions.py:935
- The wrapped form does not use the raw-close fallback: because it starts and ends with
{{ }},_is_single_expressionroutes it through the typed fast path despite the open quote. The message gives users an incorrect diagnosis; it should state that wrapping leaves a malformed expression whose result can invert the condition.
"Close the unbalanced quote first: wrapping it as written leaves the "
"quote open, so the raw-close fallback evaluates a truncated comparison "
"rather than the one written, and its result can silently invert the "
"condition instead of repairing it."
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
| * an unbalanced quote -- ``{{ inputs.name == 'abc`` corrected to | ||
| ``"{{ inputs.name == 'abc }}"``. The quote is still open, so the raw-close | ||
| fallback evaluates a truncated comparison and yields the string ``"False"``, | ||
| which ``evaluate_condition`` then reads as the ``false`` keyword. The author | ||
| pastes the correction and the condition flips from always-true to | ||
| always-false -- inverted, not repaired. |
mnriem
left a comment
There was a problem hiding this comment.
Please address Copilot feedback. Thanks for all the great work!
…ir 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.
|
Both suppressed findings were correct. Fixed in 1. The quote-balance gate was not sufficient. Same harm as the two cases this PR already covered — the author is told the condition is always true and hands back an always-false one. 2. The message named the wrong mechanism. I said the wrapped form goes through the raw-close fallback. Measured: So it takes the typed fast path instead. The diagnosis was wrong even though the conclusion — that the result can invert the condition — was right. What changedEnumerating broken shapes was the mistake; it is why this needed three rounds.
The messages now describe the text itself rather than the interpolator path it will take. Asserting an internal route is exactly what made my previous two versions wrong, twice, so they no longer do it. The test that matters
Verification — Python 3.1122 failures identical before and after — the One mutation initially escaped, and that is worth reportingRemoving Final mutation results, each gate against its own cases: operand gate → 12 failures, bracket gate → 3, dropped operator → 1. |
| for op in (" or ", " and ") + _COMPARISON_OPERATORS: | ||
| idx = _find_top_level(text, op) | ||
| if idx == -1: | ||
| continue | ||
| if not text[:idx].strip() or not text[idx + len(op):].strip(): | ||
| return True | ||
| if _find_top_level(text, "|") != -1: | ||
| if any(not segment.strip() for segment in _split_top_level(text, "|")): | ||
| return True | ||
| return text in ("not", "or", "and") or text.endswith((" not", " or", " and")) |
| def _has_unbalanced_bracket(text: str) -> bool: | ||
| """True when a bracket opened outside a quoted operand is never closed.""" | ||
| depth = 0 | ||
| 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 "([{": | ||
| depth += 1 | ||
| elif ch in ")]}": | ||
| depth -= 1 | ||
| if depth < 0: | ||
| return True | ||
| return depth != 0 |
|
Please address Copilot feedback |
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.
|
@mnriem — done, in 1. Worth naming plainly: that is the same defect this PR's parent commit fixed one level up — stopping at the first match — reintroduced inside the gate written 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 2. It now tracks opener types on a stack and rejects a non-matching closer. On the third threadThe docstring one is marked outdated and refers to the text before
So it does not attribute the inversion to the raw-close fallback; it records that earlier versions did and states the actual path, which matches what the reviewer described. I left it as the explanation for why the messages avoid naming internal routes. Happy to reword if you would rather it not mention the fallback at all. Verification — Python 3.1122 failures identical before and after — the Mutation-checked: depth-only brackets → 9 failures, first-occurrence-only → 3, dropping the end-of-core word scan → 16. |
There was a problem hiding this comment.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/specify_cli/workflows/expressions.py:1017
reason is Nonedoes not establish that wrapping repairs the condition; it only excludes four structural shapes. For example,inputs.items | lengthpasses these gates and is advertised as paste-ready, but the evaluator raisesValueError("unknown filter 'length'")for the wrapped form. The existinghe said "hi"\nthen leftfixture is also classified as correctable even though wrapping makes it resolve toNone, changing the original truthy string to false. Please validate the core against the evaluator's actual expression grammar/registered filters (or conservatively withhold a correction when that cannot be established) before returningWrap the expression.
reason = _wrapping_would_not_repair(core)
if reason is None:
return "Wrap the expression: " + format_condition_correction(condition) + "."
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…ting 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.
|
That was the strongest point raised on this PR, and it was right. Addressed in
The first replaces an always-true condition with a crash; the second inverts it. Two checks, both reading the evaluator rather than guessing
The fixture was mine, and it was wrong in two ways
Verification — Python 3.1122 failures identical before and after — the Mutation-checked: dropping either new gate fails 3 cases and nothing else. One thing worth your call, @mnriemThis is the fourth round in which the suggested correction — not the validation itself — was wrong in a new way. The rejection of never-evaluated conditions has been solid since #4182; it is the paste-ready suggestion that keeps needing another gate. I think the gates are now in the right place, because the last two read the evaluator's own tables rather than restating structure. But if you would rather not carry that surface at all, dropping the suggestion and keeping only the diagnosis would delete four helpers and remove this whole class of defect permanently. Happy to cut it down if that is the call — it is your maintenance burden, not mine. |
| unknown = _unregistered_filter(core) | ||
| if unknown is not None: | ||
| return f"it uses a filter the evaluator does not implement ({unknown!r})" | ||
| if _reads_as_prose(core): | ||
| return "it reads as several bare terms rather than one expression" | ||
| return None |
… 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.
|
Thanks for the approval @mnriem — one more push after it, because the Copilot comment that arrived two minutes later was correct and I would rather not leave it. Two more shapes got through: That is the fifth shape in four rounds, which is the argument against enumerating shapes. So this commit stops:
The probe namespace resolves roots but not leaves on purpose. I tried the permissive version first, and a namespace that answers every lookup also answers Net: two helpers fewer, and no restatement of the evaluator's tables anywhere in the gate. Verification — Python 3.1122 failures identical before and after — the Mutation-checked: dropping either check fails 6 cases and nothing else. Since this landed after your approval, please re-approve or tell me to revert to |
There was a problem hiding this comment.
Review details
Suppressed comments (1)
src/specify_cli/workflows/expressions.py:1046
- Add the second blank line before this top-level function; otherwise the repository's Ruff CI check reports E305.
def _wrapping_would_not_repair(core: str) -> str | None:
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Balanced
| not by a restatement of it. | ||
| """ | ||
| try: | ||
| _evaluate_simple_expression(text, {"inputs": _ProbeNamespace(), "steps": _ProbeNamespace()}) |
| return None | ||
|
|
||
|
|
||
| _TERM_SUFFIX = re.compile(r"(\[[^\[\]]*\])+$") |
| if any(not segment.strip() for segment in _split_top_level(stripped, "|")): | ||
| return True | ||
| return False | ||
| class _ProbeNamespace(dict): |
|
Please address Copilot feedback |
… 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.
|
@mnriem — two of the three were right and are fixed in 1. The probe rejected valid expressions — correct, and the worst finding so farBoth are valid; the first is exercised in
2. The suffix pattern was too permissive — correct
3. E305 — this one does not reproduce
The blank lines were genuinely wrong, though, and are fixed. And the comment pointed at something real that it did not name: this branch had added 3 ruff errors — two Verification — Python 3.1122 failures identical before and after — the Mutation-checked: treating every probe error as a rejection fails 2 cases; loosening the path grammar fails 2. (The first time I ran the second mutation my Standing offer, sharpenedThis is round six, and every round has been about the suggested correction, never about the validation itself — that has been correct since #4182 merged. If you would rather not carry this surface, say the word and I will cut the suggestion entirely: it deletes five helpers and ends this class permanently, leaving the diagnosis that was always the valuable part. |
There was a problem hiding this comment.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
src/specify_cli/workflows/expressions.py:970
- This still offers a paste-ready correction for malformed adjacent operators. For example,
inputs.a === inputs.bhas no empty segment when split on==, so this returnsFalse; the evaluator then treats= inputs.bas a path, resolves it toNone, and the suggested wrapped condition becomes false.inputs.a <> inputs.bfollows the same path. The validator therefore retains the inversion this change is intended to prevent. Please reject unsupported/residual operator tokens or validate recursively parsed operands before offering the correction.
This issue also appears in the following locations of the same file:
- line 1010
- line 1074
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
src/specify_cli/workflows/expressions.py:1010
- Malformed
from_jsonforms are omitted from the deterministic filter errors recognized here._apply_filterraises messages beginning withfrom_json:for inputs such asinputs.payload | from_json(), so this probe returnsNoneand the validator advertises a wrapped condition that will raise at runtime. Treat this prefix as an evaluator rejection too.
if message.startswith(("unknown filter ", "filter '")):
src/specify_cli/workflows/expressions.py:1078
- This gate does not establish that referenced paths have a valid namespace root. A bare
boguspasses the segment regex, while a compoundbogus == 'x'bypasses path checking as soon as an operator is found; the probe silently resolves both unknown roots toNone. Both therefore receive paste-ready corrections that evaluate false instead of repairing the original always-true condition. Validate variable references against the roots supplied by_build_namespace(inputs,steps,item,fan_in, andcontext) before offering a correction.
if _has_incomplete_operand(core):
return "an operator in it is missing an operand"
if _is_not_a_bare_path(core):
return "it is not a path or an expression the evaluator can resolve"
rejected = _evaluator_rejects(core)
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…rrors 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.
|
All three addressed in Every gate so far inspected only the shape it was written for, so an operand one level down walked past all of them. Recursing to the leaves
Both shapes above fall out of that without either being named, which is the property the previous rounds kept failing to reach. The filter split, done from the source this time
Under a probe, that type is the placeholder — not anything the author wrote. My previous prefix list missed the first line, and when I widened it by filter name it wrongly rejected One case no review raised
That condition was in my offered-correction fixture. It is a refusal case now. Verification — Python 3.1122 failures identical before and after — the Mutation-checked: dropping the recursion fails 15 cases, dropping the namespace-root check fails 5, treating every probe error as a rejection fails 2. (The namespace-root mutation initially reported as uncaught because my The offer still standsRound seven, still entirely about the suggested correction. I think recursing to the leaves is finally the right shape rather than another patch, because it is defined by the evaluator's grammar instead of by the examples found so far. But if you would rather not carry this surface, one word and I will cut the suggestion — five helpers go with it, and the diagnosis that was always the valuable part stays. |
|
Lets get this one through the review and then if you can describe what specifically would make it easier / cleaner / faster then lets address that by describing it in an issue so we can asses it? Hope that is OK? Thanks! |
| def _looks_numeric(text: str) -> bool: | ||
| try: | ||
| float(text) | ||
| except ValueError: | ||
| return False | ||
| return True | ||
|
|
||
|
|
||
| def _is_literal(text: str) -> bool: | ||
| if len(text) >= 2 and text[0] in ("'", '"') and text[-1] == text[0]: | ||
| return True | ||
| return text.lower() in ("true", "false", "none", "null") or _looks_numeric(text) |
| if _find_top_level(stripped, "|") != -1: | ||
| segments = _split_top_level(stripped, "|") | ||
| return _unresolvable_term(segments[0]) |
| 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" | ||
| if re.sub(r"\[\d+\]$", "", segments[0].strip()) not in _NAMESPACE_ROOTS: |
|
Please address Copilot feedback |
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.
|
@mnriem — two of the three are fixed in Fixed: my checks approximated the evaluator instead of matching itAll three are truthy before wrapping and Not fixed:
|
Follow-up to #4182. That PR merged while this last fix was in flight — by about a minute — so the defect below is still live on
main.Problem
format_condition_correctionwraps whatever it is handed. That is right for a formatter, but the three step validators advertise its output as a paste-ready correction, and for two inputs it cannot repair the suggestion is actively harmful.I measured what pasting each suggestion does rather than reasoning about it:
" ""{{ }}"{{ inputs.name == 'abc"{{ inputs.name == 'abc }}"Both invert the condition rather than repair it:
"False", whichevaluate_conditionreads as thefalsekeyword beforebool()is ever reached.So an author who follows the advice trades an always-true condition for an always-false one, having been told it was the fix.
Change
format_condition_remediationbuilds the advice, and the three validators call it instead of assembling the sentence themselves. It offers a correction only when wrapping would genuinely repair the input, and otherwise names the fault — the same call already made forcondition_has_malformed_expression_block, which deliberately offers no suggestion._has_unbalanced_quoteuses the same left-to-right scan as_find_block_closeand_strip_stray_delimiters, so "inside a string" means one thing throughout the module.Note on one of my own claims
My first draft of the unbalanced-quote message said the wrapped form "stays always true". The new test failed and showed it returns
False—evaluate_conditionrecognises the residual"False"as the keyword before coercion. The message and docstring say inverted, which is what the measurement shows. Flagging it because the distinction is the whole point of the change.Verification — Python 3.11, on this branch off
mainThe 22 failures in that run are identical with and without this commit — all
TestWorkflowCliAlignmentsymlink tests, which need elevation on Windows. I ran the suite against unmodifiedmainto confirm that rather than assume it.Mutation-checked: removing either gate fails exactly the 9 new parametrised cases and nothing else, so the fixtures pin this defect rather than passing incidentally.
Credit to the Copilot reviewer on #4182 — it raised this as a suppressed comment there and was right.