feat(workflows): parse workflow expressions with a Lark LALR grammar by markuswondrak · Pull Request #5171 · github/spec-kit

markuswondrak · 2026-10-29T12:10:56Z

Description

Closes #5060. This is the third PR of the delivery plan in the issue, after #5114 (package split) and #5130 (gap tests).

It replaces the hand-written workflow expression parser with a Lark LALR grammar (engine_lark.py) and deletes engine_current.py. Only the block-body parser changes. The template scanner, namespace resolution, filters, comparison semantics and diagnostics stay in our code.

Filter precedence from the parse tree. A filter chain cannot be an operand of an operator, in either direction. The grammar parses | as a postfix operator that binds tightest, so a filter chain is a tree node wherever it appears, and _parse refuses a tree in which an operator has one as a direct operand. count > limit | default(5) and items | length > 0 both fail with "ambiguous filter precedence", naming the operator. Parentheses keep their group node, so (items | length) > 0 is valid. Malformed text without a filter, such as inputs.mode not 'done', gets the plain positional syntax error.
One definition of path syntax. The path terminal is built from namespace._PLAIN_SEGMENT and _INDEXED_SEGMENT.
Static diagnostics. _collect_leaves and _evaluator_rejects walk the parse tree, so the probe evaluation and _leaf_sink are gone. _syntax_error reports the offending token and column.
One filter table. Filters dispatch through FILTER_TABLE, which also generates the usage text.
Dependency. lark>=1.3.1,<2.0.0 is added to the runtime dependencies. It is pure Python with no transitive dependencies. The grammar is an inline string, and the parser is built once per pair of path patterns.
Short-circuiting. and/or still skip the right operand when the left one decides, as added in fix(workflows): skip the right side of and/or when the left side decides #4923.

Deliberate behavior changes

Each change is documented in workflows/ARCHITECTURE.md and docs/reference/workflows.md, and pinned by tests.

Comparison precedence. All comparison operators share one precedence level, grouped left to right, so a == b > 0 is (a == b) > 0. The former engine's operator scan order gave them accidental priorities (==/!= looser than </>/in, repeated comparisons grouped from the right). None of the expressions in this repository combines two comparisons without and/or.
Invalid text is an error. Text that is not an expression raises ValueError instead of resolving to None or "". This covers prose inside {{ }}, path keys with characters outside [\w-], [1,2,3][1], and an unclosed quote in a condition.
Different error message. A comparison after a filter chain now reports "ambiguous filter precedence" instead of the dispatcher's "unsupported form" message. It is still a ValueError.

Forms the former engine accepted that keep working: number literals in every int()/float() form it read (.5, 5., +5, 1_000, 1.5e3), and not on the right of a comparison (a == not b). As before, a filter's call form is checked before its arguments are evaluated.

Testing

Tested locally with uv run specify --help (via the worktree venv: .venv/bin/specify --help)
Ran existing tests: uv sync --extra test && .venv/bin/python -m pytest: 13399 passed, 437 skipped
Tested with a sample project (if applicable)

Regression evidence for the parity fixes in the last commit: with only engine_lark.py reverted to the previous commit, the new tests fail (22 failures), and they pass with the change. The precedence, strict-input and malformed-number tests pass on both, because they pin the intended behavior. The short-circuit fix is covered by the existing TestExpressions::test_boolean_short_circuits: 3 of its 4 cases fail without the fix.

Regression evidence for the filter precedence fix (review round 1): with only engine_lark.py reverted, 8 of the new tests fail, and they pass with the change. The nesting and valid-form tests pass on both, because they pin the intended behavior. A fuzz comparison of 300,000 token sequences against the previous engine found the same accepted inputs and identical ASTs; messages changed only for malformed text.

AI Disclosure

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

AI disclosure: AI-generated code, tests, docs and PR text, written under the direction of @markuswondrak, who chose the design and the behavior decisions:

Grammar prototype: OpenCode (claude-opus-5.5, autonomous).
Integration, short-circuit fix and cleanup: GitHub Copilot CLI (Claude Sonnet 5.5, supervised).
Parity review, the parity fixes, and this PR description: GitHub Copilot CLI (Claude Opus 5.5, autonomous).
Review round 1: audit snapshot refresh by GitHub Copilot CLI (Claude Sonnet 5.5, autonomous); filter precedence fix by GitHub Copilot CLI (Claude Opus 5.5, autonomous).

Each commit carries its own Assisted-by: trailer.

…r hook Preparation for a grammar-based engine; behavior-neutral for the hand-written engine. - filters.py: FILTER_TABLE (function, argument form, usage text) is the single source; _REGISTERED_FILTERS, _ZERO_ARG_FILTERS and the "expected one of ..." message are derived from it. The message text is byte-identical to the previous literal in engine_current. - engine contract: _syntax_error(text) returns a parse-error message or None. engine_current has no syntax layer and always returns None. - diagnostics.py: prefers _syntax_error over the leaf walk in _unresolvable_term and after the incomplete-operand check in _wrapping_would_not_repair, so a parser's message (with position) reaches the condition diagnostics. No test changes; 1150 passed in the expression and condition test files. Assisted-by: OpenCode (model: claude-opus-5.5, autonomous)

Replace the hand-written block-body parser with a Lark (LALR) grammar and select it in engine.py. The expression language is unchanged (github#5060). - LALR parser with keyword terminal priority. - The ambiguous-precedence guards are a grammar property: a filter chain only applies to an operator-free head, so a pipe after a comparison or after `not` is a parse error ("ambiguous filter precedence"). - The PATH terminal is built from namespace._PLAIN_SEGMENT / _INDEXED_SEGMENT, so path syntax has one definition; the existing drift test now covers the grammar too. - _collect_leaves / _evaluator_rejects are a single static tree walk; the probe run and _leaf_sink are gone from the contract. - _syntax_error returns the parser's message with column, which the condition diagnostics use. - Filters dispatch through FILTER_TABLE. Deliberate test changes, each with its reason in the test: - [1,2,3][1] and an unclosed quote in a condition raise ValueError instead of silently yielding None / False. - Trailing tokens after a filter name, and a comparison after a filter chain, are parse errors ("invalid expression") instead of dispatcher messages; still ValueError. - Three diagnostics fixture rows expect the parser's message. - The probe-only tests are replaced (leaf-walk transparency of groups) or removed (_leaf_sink); imports of engine internals are dropped. Assisted-by: OpenCode (model: claude-opus-5.5, autonomous) Assisted-by: GitHub Copilot CLI (model: Claude Sonnet 5.5, supervised) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

The Lark engine evaluated both operands of `and`/`or`, which regressed the behavior added in github#4923: a guard such as `inputs.payload != '' and (inputs.payload | from_json | length)` ran `from_json` on an empty string again. Return as soon as the left operand decides the result. Validation walks the parse tree, so skipped operands are still checked there. Assisted-by: GitHub Copilot CLI (model: Claude Sonnet 5.5, supervised) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

The Lark engine is the only engine behind engine.py, so delete engine_current.py and everything that only it used: the `_leaf_sink` export, `_split_top_level_commas`, `_is_single_list_literal`, `_is_wrapped_in_parens`, `_EXPR_PATTERN` and `_ZERO_ARG_FILTERS`. Rename engine_lark_a2 to engine_lark, pin `lark>=1.3.1,<2.0.0`, and drop the prototype wording from docstrings and test comments. Assisted-by: GitHub Copilot CLI (model: Claude Sonnet 5.5, supervised) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…k engine Review of the Lark engine against the former hand-written engine found inputs that now fail or report the wrong problem: - Number literals accepted by int()/float() before (`.5`, `5.`, `+5`, `1_000`, `1.5e3`) failed to lex. `1e3` stays invalid, as before. - `a == not b` no longer parsed; `not` is accepted on the right of a comparison again and negates only its operand. - Filters evaluated their arguments before checking the call form, so a failing argument hid a misused or unknown filter. The documented order (form first) is restored. - Condition validation reported an operator after a filter chain (`items | length > 0`) as "missing an operand", and `a || b` as "ambiguous filter precedence". The parser's expected-token set now separates the two. Documented as deliberate and pinned by tests: - All comparison operators share one precedence level, left to right (`a == b > 0` is `(a == b) > 0`). The former engine's scan order gave them accidental priorities. - Prose in `{{ }}` and path keys outside `[\w-]` are syntax errors instead of a None lookup. Assisted-by: GitHub Copilot CLI (model: Claude Opus 5.5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

Copilot

🟡 Changes recommended

The new dependency is missing from the security-audit snapshot, which will fail CI, and malformed non-filter expressions receive misleading filter-precedence advice.

2 open issues

_{Review effort: Max (auto)}

mnriem · 2026-10-29T13:26:08Z

Please address Copilot feedback and CI/CD errors

Regenerated with the command documented in CONTRIBUTING.md so the dependency-audit job finds lark in the committed snapshot. The command upgrades, so a few unrelated pins moved to their latest patch releases. Assisted-by: GitHub Copilot (model: Claude Sonnet 5.5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

`inputs.mode not 'done'` was reported as an ambiguous filter precedence mistake although it has no filter. The message was inferred from the terminals the parser expected at the failure, and `|` is expected after any plain operand, not only after a filter chain. The grammar now parses `|` as a postfix operator that binds tightest, so a filter chain is a node wherever it appears. `_parse` refuses a tree in which an operator has such a node as a direct operand and names the leftmost one. Parentheses keep their group node, so `(items | length) > 0` stays valid. Accepted inputs and their ASTs are unchanged; malformed text without a misplaced chain now gets the plain positional syntax error. Assisted-by: GitHub Copilot (model: Claude Opus 5.5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

markuswondrak · 2026-10-30T00:45:01Z

Review round 1: both findings addressed

Audit snapshot (d17ae13): regenerated .github/security-audit-requirements.txt with the command in CONTRIBUTING.md; it now pins lark==1.3.1. The command upgrades, so a few unrelated pins moved to their latest patch releases.
Misleading filter precedence advice (3999261): confirmed. inputs.mode not 'done' and any <operand> not … got the filter message, because it was inferred from the expected terminals, and | is expected after every plain operand. Instead of tightening that inference, the grammar now parses | as a tightest-binding postfix operator, and _parse refuses a tree where an operator has a filter chain as a direct operand, naming the leftmost operator. Accepted inputs and ASTs are unchanged (fuzz comparison against the previous engine, 300,000 inputs). New positive and negative tests cover nesting in parentheses, lists and filter arguments; 8 of them fail without the fix.

Full suite: 13399 passed, 437 skipped. ruff@0.15.0 check src tests clean.

AI disclosure: posted on behalf of @markuswondrak by GitHub Copilot CLI. Snapshot refresh: Claude Sonnet 5.5, autonomous. Precedence fix, tests and this comment: Claude Opus 5.5, autonomous; the grammar-based approach was chosen by @markuswondrak.

Copilot

🟡 Changes recommended

The documentation overstates validation-time syntax checking, and expression-error diagnostics omit the offending operator and parenthesization guidance.

3 open issues

1 resolved since last review

Require CMP_OP absence for filter-precedence diagnostics

_{Review effort: Max (auto)}

mnriem · 2026-10-30T16:22:40Z

Please address Copilot feedback

…rect validation docs The operator-first filter-precedence error now names the offending operator and suggests parenthesizing the filtered operand, matching the chain-first message. ARCHITECTURE.md no longer claims condition expressions are syntax-checked at validation time; validators check only delimiters and layout. Assisted-by: GitHub Copilot (model: Claude Sonnet 5.5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

markuswondrak · 2026-10-31T12:24:30Z

Review round update, commit 8c6ed22.

The filter-precedence error for an operator whose operand is filtered (count > limit | default(5), not x | default(1)) now names the operator and suggests parenthesizing the filtered operand, like the chain-first message. Tests cover > and not and fail on the previous message.
workflows/ARCHITECTURE.md no longer says condition expressions are syntax-checked at validation time. Validators check only {{ }} delimiters and layout, so {{ inputs.count + 1 }} passes validation and fails when the step runs. Validating expression syntax in the step validators is left for a separate change.
The audit snapshot finding was fixed earlier in d17ae13; lark==1.3.1 is in .github/security-audit-requirements.txt and recompiling it produces the same pins.

Workflow and expression tests pass locally (3210 passed, 30 skipped); ruff@0.15.0 is clean.

AI disclosure: posted on behalf of @markuswondrak by GitHub Copilot CLI (model: Claude Sonnet 5.5, autonomous). The code, documentation, commit and this comment were AI-generated; the contributor has not independently reviewed this round's change.

Markus and others added 5 commits October 29, 2026 11:32

markuswondrak requested a review from mnriem as a code owner October 29, 2026 12:10

Copilot AI auto review requested due to automatic review settings October 29, 2026 12:10

Copilot started reviewing on behalf of markuswondrak October 29, 2026 12:11

View session

Copilot AI reviewed Oct 29, 2026

mnriem requested an auto review from Copilot October 29, 2026 12:16

Copilot started reviewing on behalf of mnriem October 29, 2026 12:16

View session

Copilot AI reviewed Oct 29, 2026

View reviewed changes

Comment thread pyproject.toml Resolved

Comment thread src/specify_cli/workflows/expressions/engine_lark.py Resolved

mnriem added the triage-nice-to-have Verdict: evidence-backed fix or greenlit feature — land after review label Oct 29, 2026

mnriem added the author-awaiting Waiting on author response label Oct 29, 2026

Markus and others added 2 commits October 30, 2026 01:43

Copilot AI auto review requested due to automatic review settings October 30, 2026 00:44

Copilot started reviewing on behalf of markuswondrak October 30, 2026 00:44

View session

Copilot AI reviewed Oct 30, 2026

mnriem requested an auto review from Copilot October 30, 2026 13:35

Copilot started reviewing on behalf of mnriem October 30, 2026 13:36

View session

Copilot AI reviewed Oct 30, 2026

View reviewed changes

Comment thread src/specify_cli/workflows/expressions/engine_lark.py Resolved

Comment thread workflows/ARCHITECTURE.md Resolved

markuswondrak force-pushed the feat/5060-expression-lark-engine branch from f683475 to 8c6ed22 Compare October 31, 2026 12:23

Navigation Menu

Repository navigation

feat(workflows): parse workflow expressions with a Lark LALR grammar - #5171

feat(workflows): parse workflow expressions with a Lark LALR grammar#5171
markuswondrak wants to merge 8 commits into
github:mainfrom
markuswondrak:feat/5060-expression-lark-engine

markuswondrak commented Oct 29, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

mnriem commented Oct 29, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

markuswondrak commented Oct 30, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

mnriem commented Oct 30, 2026

Uh oh!

markuswondrak commented Oct 31, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

markuswondrak commented Oct 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Deliberate behavior changes

Testing

AI Disclosure

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

mnriem commented Oct 29, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

markuswondrak commented Oct 30, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

mnriem commented Oct 30, 2026

Uh oh!

markuswondrak commented Oct 31, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

markuswondrak commented Oct 29, 2026 •

edited

Loading