Skip to content

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

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

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
@mnriem
mnriem requested an auto review from Copilot November 2, 2026 15:03
@mnriem

mnriem commented Nov 2, 2026

Copy link
Copy Markdown
Collaborator

Blocked on #5060

Once it lands re-evaluate to see if it is still relevant

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Whitespace around dots passes validation but is not normalized by the resolver, causing expressions to silently return None and potentially misdirect conditions.

1 open issue
What changed in this PR

This PR addresses #5073 by extending Spec Kit’s workflow expression evaluator and rejecting unsupported syntax rather than silently treating it as missing data.

Changes:

  • Supports exponent-form numbers and chained list indexing.
  • Adds runtime errors with hints for unsupported syntax while preserving validation-probe behavior.
  • Adds regression coverage and documents the expression behavior.
File Description
tests/​unit/​test_condition_expression_block.py Updates diagnostic expectations for the new evaluator behavior.
tests/​test_workflows.py Adds regression coverage for numeric literals, indexing, and syntax errors.
src/​specify_cli/​workflows/​expressions/​operators.py Coerces exponent-form numeric strings in comparisons.
src/​specify_cli/​workflows/​expressions/​namespace.py Resolves chained list indices.
src/​specify_cli/​workflows/​expressions/​engine_current.py Parses exponent literals and rejects unsupported expression syntax.
docs/​reference/​workflows.md Documents supported forms and runtime syntax errors.

Review effort: Balanced (auto)

if (
expr
and not isinstance(namespace, _ProbeNamespace)
and not all(_is_path_segment(seg.strip()) for seg in expr.split("."))
@mnriem

mnriem commented Nov 2, 2026

Copy link
Copy Markdown
Collaborator

Please address Copilot feedback. If not applicable, please explain why

@mnriem mnriem added the author-awaiting Waiting on author response label Nov 2, 2026
@aarjang

aarjang commented Nov 9, 2026

Copy link
Copy Markdown
Author

Re-evaluated against main now that #5060 has landed via #5171, as requested. The Lark engine supersedes most of this PR, so I am closing it.

What the new grammar already covers, verified against current main:

  • Case 2 (escaped quote in a string literal): {{ 'it\'s' }} and comparisons against it now raise a clear syntax error naming the position — satisfies the issue's "evaluate or raise".
  • Case 3 (chained indexing): {{ steps.plan.output.matrix[0][1] }} is rejected as a syntax error — also "evaluate or raise", though Jinja2 would resolve it.
  • Case 4 (word operators without spaces): {{ inputs.a=='x'and inputs.b }} and {{ 'x'in inputs.a }} now parse and evaluate correctly.

The remaining delta from #5073 is case 1 only: {{ 1e3 }} raises invalid expression '1e3': unexpected 'e3' at column 2 instead of evaluating to 1000.0, and a numeric string in exponent form does not coerce in comparisons ({{ inputs.s > 100 }} with s = "1e3" is False), while workflow input coercion accepts "1e3" as 1000 — so the same text still means different things in different places. If that residue is worth fixing, I can send a small focused PR against the grammar and _coerce_number; otherwise #5073 can likely be closed as addressed by #5171.

The Copilot finding on this PR (whitespace around dots) applied to the now-removed hand-written engine's guard and is moot with the close; the Lark path terminal rejects those shapes outright.

AI disclosure: this comment and the analysis behind it were produced with Claude Code (model: Claude Fable 5, default reasoning effort, human-supervised), posted on behalf of @aarjang, who reviewed it before posting.

@aarjang aarjang closed this Nov 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author-awaiting Waiting on author response 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

3 participants