Skip to content

fix(workflows): evaluate exponent numbers and chained indexing, reject unsupported syntax - #5206

Open
aarjang wants to merge 1 commit into
github:mainfrom
aarjang:fix/5073-silent-none-expressions
Open

aarjang wants to merge 1 commit into
github:mainfrom
aarjang:fix/5073-silent-none-expressions

Conversation

@aarjang

@aarjang aarjang commented Nov 1, 2026

Copy link
Copy Markdown

Description

Fixes #5073.

When the workflow expression evaluator met syntax it did not parse, it treated the text as a variable path, which resolved to None — so a condition using such syntax quietly gave the wrong result. The issue confirmed four cases: exponent-form numbers (1e3), backslash-escaped quotes in string literals ('it\'s'), chained indexing (matrix[0][1]), and word operators without surrounding spaces ('x'and inputs.b).

This PR makes each case either evaluate the way Python and Jinja2 do or fail loudly, in line with the module's documented contract ("a safe Jinja2 subset") and the precedent of mis-wired filters, which already raise ValueError:

  • Case 1 — exponent numbers evaluate. Numeric literals parse with int() first, then float() for digit-bearing text, so {{ 1e3 }} is 1000.0. The words float() also accepts (inf, nan) carry no digit and still resolve as variable names. _coerce_number applies the same rule, so {{ inputs.s > 100 }} with s = "1e3" is true — matching what workflow input coercion already accepts as a number, so the same text no longer means different things in different places.
  • Case 3 — chained indexing resolves. _INDEXED_SEGMENT accepts a chain of indices and _resolve_dot_path walks them, so {{ steps.plan.output.matrix[0][1] }} is "b". An out-of-range index is missing data, not syntax, and still resolves to None. The diagnostics gate reads the shared pattern, so validation follows for free (the existing wiring test pins this).
  • Cases 2 and 4 — unsupported syntax raises. Text that is neither a literal nor a resolvable variable path raises ValueError naming the unsupported syntax, with a targeted hint for the two common shapes: an escaped quote in a string literal (quote it with the other quote type) and an unspaced word operator (and/or/in/not need a space on both sides). A well-formed path that merely finds nothing (inputs.missing) still resolves to None: missing data is not a syntax fault.

Validation probes (_collect_leaves, _evaluator_rejects) are exempt from the new raise, so the authoring diagnostics and their messages are unchanged — the gate still owns validation-time wording, and all existing diagnostics tests pass unmodified except the five that deliberately pinned the old evaluator behavior through the gate (they now pin the new behavior, which is exactly the drift-detection those pins exist for).

The docs gain a note in the expression section describing the new error and the newly supported forms.

Testing

  • Three new regression tests in TestExpressions (one per issue theme) fail on main exactly as the issue describes — {{ 1e3 }} is None, matrix[0][1] is None, escaped quotes and unspaced operators silently yield None/False — and pass with this change.

  • The reproduction script from [Bug]: Unsupported expression syntax evaluates to None silently instead of raising #5073 now prints the expected value for every case: cases 1 and 3 evaluate as in Jinja2, cases 2 and 4 raise ValueError naming the syntax, and input coercion of "1e3" agrees with expression evaluation.

  • tests/test_workflows.py + tests/unit/test_condition_expression_block.py + tests/workflows: 1,808 pass locally (.venv/bin/python -m pytest per CONTRIBUTING).

  • Full suite: 13,650 passed; the same 11 pre-existing failures in the script-parity tests (test_*_python_parity.py) occur on a clean main checkout on this machine and each passes in isolation on both branches (also noted in fix(workflows): evaluate chained comparisons pairwise like Python #5203).

  • Tested locally with uv run specify --help

  • Ran existing tests with uv sync && uv run pytest

  • Tested with a sample project (if applicable)

AI Disclosure

  • I did not use AI assistance for this contribution
  • I did use AI assistance (fill in the disclosure below)

AI disclosure: Written with Claude Code (model: Claude Fable 5, default reasoning effort, human-supervised). The agent located the defects, wrote the fix, tests and docs note, and ran the verification (failing-on-main tests, issue repro script, full-suite comparison against clean main); I reviewed the diff and the evidence before submitting. The commit carries a matching Assisted-by: trailer.

🤖 Generated with Claude Code

…t unsupported syntax

When the expression evaluator met syntax it did not parse, it treated
the text as a variable path, which resolved to None, so a condition
using such syntax quietly gave the wrong result. Four confirmed cases:
exponent-form numbers (1e3), backslash-escaped quotes in string
literals ('it\'s'), chained indexing (matrix[0][1]), and word operators
without surrounding spaces ('x'and inputs.b).

Make each case either evaluate the way Python and Jinja2 do or fail
loudly, in line with the module's documented contract:

- Numeric literals parse with int() first, then float() for
  digit-bearing text, so 1e3 is 1000.0; the words float() also accepts
  (inf, nan) still resolve as variable names. _coerce_number applies
  the same rule, so a numeric string in exponent form compares as a
  number, matching workflow input coercion.
- Indexed path segments accept a chain of indices, so matrix[0][1]
  walks both. An out-of-range index is missing data, not syntax, and
  still resolves to None.
- Text that is neither a literal nor a resolvable variable path raises
  ValueError naming the unsupported syntax, the way mis-wired filters
  already do, with a hint for the two common shapes (escaped quote,
  unspaced word operator). Validation probes are exempt, so authoring
  diagnostics and their messages are unchanged.

Fixes github#5073

Assisted-by: Claude Code (model: Claude Fable 5, supervised)
@aarjang
aarjang requested a review from mnriem as a code owner November 1, 2026 07:48
@mnriem mnriem added the triage-must-have Verdict: high-value, important work for Spec Kit — do first label Nov 1, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

triage-must-have Verdict: high-value, important work for Spec Kit — do first

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Unsupported expression syntax evaluates to None silently instead of raising

2 participants