fix(workflows): reject a condition that has no {{ }} block

[ntdatt812]

Defect

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.

With inputs.count == 5:

evaluate_condition("{{ inputs.count > 100 }}", ctx)   # False  — correct
evaluate_condition("inputs.count > 100", ctx)         # True   — never evaluated
evaluate_condition("inputs.name == 'zzz'", ctx)       # True
evaluate_condition("inputs.count < 3", ctx)           # True

Every brace-less condition is true, whatever it says. An if step always takes then; a while/do-while step never terminates on its condition and runs to max_iterations — ten agent invocations for a loop the author expected to stop after one.

Nothing reports it. The workflow validates, runs, and takes the wrong branch silently.

Why this is worth a validation error

The three step validators already reject a list/dict/number condition, and the comment there states the reason exactly:

a list/dict/number condition silently resolves to its truthiness (e.g. condition: [1, 2] is always True) with no error, branching wrongly on an authoring mistake. Reject those at validation […]

A brace-less string is the same failure mode, and a likelier mistake: GitHub Actions accepts a bare expression in if: (if: github.event_name == 'push'), so an author arriving from Actions writes the brace-less form by habit — and unlike [1, 2], condition: inputs.count > 100 looks completely correct on the page.

Fix

condition_is_never_evaluated() in expressions.py, wired into the if, while and do-while validators. The error names the problem and hands back the corrected form:

If step 's1': 'condition' 'inputs.count > 100' has no '{{ }}' block, so it is
never evaluated and is always true. Wrap the expression: "{{ inputs.count > 100 }}".

Validation only — no runtime behaviour changes. Still valid, and covered by tests: "{{ ... }}" in any position, "true"/"false" in any case, real bools, empty and whitespace strings, and every non-string type (already handled by the branch above).

Tests

New tests/unit/test_condition_expression_block.py, 40 cases:

• the runtime behaviour is pinned first — same expression with and without braces, asserting False vs True — so the defect stays documented even if the validator changes
• each of the three step validators rejects the brace-less form and echoes the corrected expression
• no false positives, parametrised across all three step types
• the helper itself across strings, bools, containers and numbers

tests/unit + tests/test_workflows.py

before:  22 failed, 1083 passed, 9 skipped
after:   22 failed, 1123 passed, 9 skipped

The same 22 pre-existing failures in both runs — all symlink_to on Windows without the privilege (OSError: [WinError 1314]), unrelated to this change. Everything added is the +40 new tests.

[Copilot]

Pull request overview

Adds validation to reject brace-less workflow conditions that would otherwise always evaluate as true.

Changes:

• Adds a shared condition-validation helper.
• Integrates validation into if, while, and do-while steps.
• Adds runtime and validator regression tests.

Show a summary per file

File	Description
src/specify_cli/workflows/expressions.py	Adds condition detection helper.
src/specify_cli/workflows/steps/if_then/__init__.py	Validates if conditions.
src/specify_cli/workflows/steps/while_loop/__init__.py	Validates while conditions.
src/specify_cli/workflows/steps/do_while/__init__.py	Validates do-while conditions.
tests/unit/test_condition_expression_block.py	Adds regression coverage.

Review details

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

• Files reviewed: 5/5 changed files
• Comments generated: 4
• Review effort level: Balanced

[ntdatt812]

Both review points were correct — thanks. Fixed in c0c291c.

1. Unterminated / reversed delimiters slipped through

Confirmed against the interpolator rather than assumed. _interpolate_expressions() takes its raw_close == -1 branch and appends the tail verbatim, so nothing is substituted:

condition	evaluate_condition	old helper	new helper
inputs.count > 100	True	flags	flags
{{ inputs.count > 100	True	misses	flags
}} inputs.count > 100 {{	True	misses	flags
{{ inputs.count > 100 }}	False	—	—

The helper now requires a complete block: it finds the first {{ and checks a }} follows it. {{ a }} {{ b }} and {{ inputs.text | default('}}') }} stay unflagged.

2. The correction was not valid YAML

Reproduced exactly as described — condition: "{{ inputs.name == "zzz" }}" raises yaml.parser.ParserError. A remediation the author cannot paste is not a remediation.

Added format_condition_correction() in expressions.py, used by all three validators as suggested. It picks quoting from the content — double by default, single when the expression holds a double quote, double with backslash escapes when it holds both — and drops a stray delimiter instead of nesting a second one, so {{ inputs.count > 100 corrects to "{{ inputs.count > 100 }}" rather than "{{ {{ ... }} }}".

condition	emitted correction
inputs.name == "zzz"	'{{ inputs.name == "zzz" }}'
inputs.name == 'zzz'	"{{ inputs.name == 'zzz' }}"
inputs.a == "x" and inputs.b == 'y'	"{{ inputs.a == \"x\" and inputs.b == 'y' }}"
inputs.path == 'C:\tmp'	"{{ inputs.path == 'C:\tmp' }}"

Tests — the file goes from 40 to 70 cases. The new ones cover the incomplete forms, and run every correction through yaml.safe_load asserting it loads back to exactly {{ <expr> }} and that the corrected form is not itself re-flagged — the invariant that would have caught both of these.

uvx ruff@0.15.0 check src tests clean; pytest tests/unit 250 passed (the 2 symlink failures are a local Windows privilege limitation, green on CI).

[Copilot]

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 2
• Review effort level: Balanced

[mnriem]

Please address Copilot feedback

[ntdatt812]

@mnriem — both follow-up findings addressed in dc3c7de. Reproduced each one first rather than taking them on trust; both were real.

1. The }} check was not quote-aware

Confirmed exactly as described:

condition	evaluate_condition	validator, before	after
{{ inputs.x == '}}'	True	misses it	flags
{{ inputs.x == '}}' }}	False	—	not flagged
{{ inputs.text | default('}}') }}	—	not flagged	not flagged

_interpolate_expressions finds no quote-aware close, falls to its raw-close branch, evaluates a truncated body and leaves residual text (False') — non-empty, so bool() makes it true.

Rather than restate the quote rules a third time in this module, I lifted the scan out of _interpolate_expressions into _find_block_close() and had the validator call it. The check and the substitution it is predicting now share one implementation and cannot drift — which is the same failure mode this PR is about. _interpolate_expressions keeps its exact previous behaviour; the extraction is behaviour-preserving.

2. Hand-rolled quoting missed control characters

Also reproduced — a condition carrying a newline did not round-trip:

condition	before	after
inputs.x == 1\nand inputs.name == 'abc'	newline folded, round-trip fails	exact
he said "hi"\nthen left	ParserError	exact
inputs.a == 'x\ry'	round-trip fails	exact

Switched to json.dumps(..., ensure_ascii=False) as suggested — json was already imported, every JSON string is a valid YAML double-quoted scalar, and it removes the escape enumeration entirely. ensure_ascii=False keeps a non-ASCII operand readable instead of expanding it into \uXXXX; there is a test for that.

Tests 70 → 83. The quoted-delimiter condition joins the incomplete-block set; the round-trip set gains multiline, newline-with-quote, tab, carriage return and a non-ASCII operand. All four new cases fail on the previous commit (4 failed, 79 passed) and pass here (83 passed) — they are not vacuous.

Verification on this head: uvx ruff@0.15.0 check src tests — All checks passed. pytest tests/unit — 263 passed. pytest tests/workflows — 179 passed. The handful of local failures in both directories are the Windows symlink-privilege cases, identical on unpatched main; the CI matrix is the authority there and was green on the previous head.

[Copilot]

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/specify_cli/workflows/expressions.py:730

• Non-empty whitespace is not false at runtime. evaluate_condition() strips only while checking the true/false keywords and then falls through to bool(result), so bool(" ") is True. Excluding it here lets every validator accept a condition that silently takes then or loops to max_iterations, exactly the failure this helper is intended to reject. Please distinguish the truly empty string from non-empty whitespace (and update the tests that currently mark whitespace valid), or normalize whitespace in the runtime evaluator.

This issue also appears on line 765 of the same file.

    stripped = condition.strip()
    if not stripped or stripped.lower() in ("true", "false"):
        return False

src/specify_cli/workflows/expressions.py:768

• The correction only removes delimiters at the edges, but the validator also flags malformed delimiters in the middle. For example, prefix {{ inputs.ready is flagged and corrected to "{{ prefix {{ inputs.ready }}"; likewise inputs.ready }} suffix retains the inner }}. These suggestions do not evaluate the intended condition and may subsequently evade this validator because they contain a complete outer/first block. Please either sanitize stray delimiters outside quoted operands or avoid offering an automatic correction for malformed-block cases.

    core = str(condition).strip()
    core = re.sub(r"^\s*(\{\{|\}\})\s*", "", core)
    core = re.sub(r"\s*(\{\{|\}\})\s*$", "", core).strip()
    return json.dumps("{{ " + core + " }}", ensure_ascii=False)

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[ntdatt812]

Both suppressed findings in the latest Copilot pass are real. Fixed in aa9434a, and I reproduced each one against the committed code before touching it rather than taking them on trust.

1. Non-empty whitespace was excluded, and it should not have been

My docstring claimed a whitespace condition "coerces to False, which is a definite answer". That holds only for the empty string:

evaluate_condition("")      -> False
evaluate_condition("   ")   -> True
evaluate_condition("\t\n ") -> True

evaluate_condition strips only while testing the true/false keywords and then falls through to bool() on the raw string — and this repo pins that deliberately in test_condition_whitespace_only_string_stays_truthy. So condition: " " is precisely the silent always-true this helper exists to catch, and it was walking past the validator.

That existing test is why I fixed it at validation time rather than normalizing in the evaluator, which was Copilot's alternative: normalizing would contradict a behaviour the suite intentionally holds. The empty string stays excluded, because it really does coerce to False.

2. The correction removed only edge delimiters, so it could nest one

"prefix {{ inputs.ready"  ->  "{{ prefix {{ inputs.ready }}"

The suggestion carried an unclosed inner block, and because the outer block was complete, condition_is_never_evaluated accepted the corrected form — the validator waving through its own bad advice. Same shape with 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 the blanket re.sub Copilot's first phrasing suggests would eat it and silently change what the condition compares against. I could not reuse _find_top_level — it counts { and } as bracket depth, so it never reports a {{ as a top-level token at all.

"prefix {{ inputs.ready"    -> "{{ prefix inputs.ready }}"
"inputs.ready }} suffix"    -> "{{ inputs.ready suffix }}"
"{{ inputs.x == '}}'"       -> "{{ inputs.x == '}}' }}"        <- delimiter kept as data
'{{ inputs.name == "a  b"'  -> '{{ inputs.name == "a  b" }}'   <- inner spacing kept

Whitespace collapses only where a delimiter was removed; inside a quoted operand nothing is touched.

One consequence I would rather name than leave for you to find. For a whitespace-only condition there is nothing to wrap, so the message ends Wrap the expression: "{{ }}". The diagnosis is right and the suggestion is empty. If you would rather the three validators omit the correction clause when there is no content to wrap, say so and I will thread that through — I left it alone because it means editing the message construction in if_then, while_loop and do_while, and I did not want to widen the diff while changes are requested.

Verification. tests/unit/test_condition_expression_block.py: 87 passed. The two fixtures that asserted whitespace was valid are corrected, and five cases added for interior delimiters, quoted delimiters and quoted spacing.

tests/test_workflows.py is 20 failed / 903 passed with this change, and 20 failed / 903 passed with it stashed — identical. All twenty are symlink tests that need Windows Developer Mode. Repo-wide the run is 168 failed / 6775 passed, and none of the 168 touches conditions or expressions; the same environment limit accounts for them.

@mnriem — this is on top of the two findings you raised, which went in as dc3c7de. As an outside contributor I cannot re-request review, so this comment is the only signal I have.

[Copilot]

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/specify_cli/workflows/expressions.py:752

• This does not always mean the condition is “never evaluated.” _interpolate_expressions falls back to the first raw }} when the quote-aware scan fails and evaluates the truncated body. For example, {{ inputs.missing | default('oops }} reaches _apply_filter and raises ValueError, while this helper returns True, causing all three validators to report that it is always true. Please distinguish genuinely uninterpolated text from malformed blocks that take the raw-close evaluation path, and report the latter as malformed rather than always true.

    return _find_block_close(stripped, open_at) == -1

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Balanced

[ntdatt812]

Third finding, and it holds too. Fixed in 3e28b32, reproduced before changing anything.

condition_is_never_evaluated was returning True for any {{ the quote-aware scan could not close — but _interpolate_expressions does not treat those alike, and its own comment says so. Two sub-cases, only one of which is "never evaluated":

{{ inputs.count > 100                  -> True             no raw '}}': emitted verbatim
}} inputs.count > 100 {{               -> True             same
{{ inputs.x == '}}'                    -> True             raw-close path, residual text
{{ inputs.missing | default('oops }}   -> raises ValueError

That last one made the message wrong on both halves: it is evaluated, and it does not end up true — it ends the run in _apply_filter.

Added condition_has_malformed_expression_block with its own branch in the three validators, because the two faults want opposite advice: one says you forgot the braces, the other says your delimiters or quotes do not balance. The predicates are mutually exclusive, pinned by a test over every fixture rather than left as an assumption.

The malformed branch deliberately offers no correction, which closes the loop on your earlier point about not auto-correcting malformed-block cases — it applies exactly here. The fault is unbalanced quoting, so the quote-aware stripper cannot tell operand from delimiter: for {{ inputs.missing | default('oops }} it produces

"{{ inputs.missing | default('oops }} }}"

which is not a fix. Naming the fault beats handing back something that looks authoritative and is not.

That also resolves the loose end I flagged last round: a blank correction now renders "{{ }}" instead of the double-spaced "{{ }}", so the whitespace case reads properly without needing to restructure the message.

Verification. 106 pass in tests/unit/test_condition_expression_block.py. Across tests/test_workflows.py + tests/unit: 22 failed / 1189 passed with this change, and 22 failed / 1170 passed with the diff stashed — the same 22, all Windows symlink cases needing Developer Mode, none touching conditions or expressions. The passed delta is exactly the tests added here.

@mnriem — that is three review rounds of findings now addressed (c0c291c, dc3c7de, aa9434a, 3e28b32). I still cannot re-request review as an outside contributor, so this comment is the only signal available to me.

[Copilot]

Review details

Suppressed comments (1)

src/specify_cli/workflows/expressions.py:787

• Only the first expression block is inspected. A condition such as {{ inputs.name }} {{ inputs.missing | default('oops }} is accepted because the first block closes, although the later malformed block raises ValueError at runtime (the behavior is already pinned in tests/test_workflows.py:301-319). Scan every opener so malformed later blocks fail validation too.

    open_at = stripped.find("{{")
    if open_at == -1:
        return False
    if _find_block_close(stripped, open_at) != -1:
        return False
    return stripped.find("}}", open_at + 2) != -1

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Balanced