Repository navigation
feat(workflows): parse workflow expressions with a Lark LALR grammar - #5171
Open
markuswondrak wants to merge 7 commits into
Open
markuswondrak wants to merge 7 commits into
markuswondrak wants to merge 7 commits into
Conversation
…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>
| "json5>=0.13.0", | ||
| "mcp>=2.2.0,<3.0.0", | ||
| "pydantic>=2.13.0,<3.0.0", | ||
| "lark>=1.3.1,<2.0.0", |
mnriem
added
the
triage-nice-to-have
Collaborator
|
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>
Contributor
Author
|
Review round 1: both findings addressed
Full suite: 13399 passed, 437 skipped. 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. |
Comment on lines
+304
to
+310
| if not exc.chain_first: | ||
| return ( | ||
| f"ambiguous filter precedence in '{text}': a filter cannot be an " | ||
| f"operand of an operator. Filter the operand in its own expression " | ||
| f"instead." | ||
| ) | ||
| operator = " ".join(str(exc.operator).split()) |
|
|
||
| **Operator precedence.** From loosest to tightest: `or`, `and`, `not`, then the comparisons (`==`, `!=`, `>`, `<`, `>=`, `<=`, `in`, `not in`). All comparisons share **one** level and group left to right, so no comparison has priority over another: `{{ a == b > 0 }}` is `{{ (a == b) > 0 }}`, and `{{ 1 < 2 < 3 }}` is `{{ (1 < 2) < 3 }}`. The former hand-written engine gave comparisons accidental priorities through its operator scan order; parenthesize, or join comparisons with `and`/`or`, instead of relying on either reading. A `not` on the right of a comparison negates only its operand: `{{ a == not b }}`. | ||
|
|
||
| **Only expressions parse.** The body of every `{{ }}` block is parsed by a grammar, so text that is not an expression is a `ValueError` naming the offending token and column: prose such as `{{ some text }}`, path keys with characters outside letters, digits, `_` and `-` (`{{ inputs.weird:key }}`), arithmetic, or two adjacent operands. The former engine looked such text up as a path and rendered the miss as an empty string or `None`. Conditions are checked at validation time; other fields fail when the step runs. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 deletesengine_current.py. Only the block-body parser changes. The template scanner, namespace resolution, filters, comparison semantics and diagnostics stay in our code.|as a postfix operator that binds tightest, so a filter chain is a tree node wherever it appears, and_parserefuses a tree in which an operator has one as a direct operand.count > limit | default(5)anditems | length > 0both fail with "ambiguous filter precedence", naming the operator. Parentheses keep their group node, so(items | length) > 0is valid. Malformed text without a filter, such asinputs.mode not 'done', gets the plain positional syntax error.namespace._PLAIN_SEGMENTand_INDEXED_SEGMENT._collect_leavesand_evaluator_rejectswalk the parse tree, so the probe evaluation and_leaf_sinkare gone._syntax_errorreports the offending token and column.FILTER_TABLE, which also generates the usage text.lark>=1.3.1,<2.0.0is 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.and/orstill 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.mdanddocs/reference/workflows.md, and pinned by tests.a == b > 0is(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 withoutand/or.ValueErrorinstead of resolving toNoneor"". This covers prose inside{{ }}, path keys with characters outside[\w-],[1,2,3][1], and an unclosed quote in a condition.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), andnoton the right of a comparison (a == not b). As before, a filter's call form is checked before its arguments are evaluated.Testing
uv run specify --help(via the worktree venv:.venv/bin/specify --help)uv sync --extra test && .venv/bin/python -m pytest: 13399 passed, 437 skippedRegression evidence for the parity fixes in the last commit: with only
engine_lark.pyreverted 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 existingTestExpressions::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.pyreverted, 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
AI disclosure: AI-generated code, tests, docs and PR text, written under the direction of @markuswondrak, who chose the design and the behavior decisions:
Each commit carries its own
Assisted-by:trailer.