fix: interpolate multi-expression templates instead of returning None (#3208)#3228
Open
Noor-ul-ain001 wants to merge 2 commits into
Open
fix: interpolate multi-expression templates instead of returning None (#3208)#3228Noor-ul-ain001 wants to merge 2 commits into
Noor-ul-ain001 wants to merge 2 commits into
Conversation
…github#3208) `evaluate_expression` returned None for templates containing two or more `{{ }}` blocks with no surrounding literal text, e.g. `"{{ context.run_id }} {{ inputs.issue }}"`. The single-expression fast path used `_EXPR_PATTERN.fullmatch()`, but `fullmatch` defeats the pattern's non-greedy `(.+?)` body: for two adjacent expressions it still matches, capturing everything between the first `{{` and the last `}}` (`"context.run_id }} {{ inputs.issue"`) as the body. That garbage failed dot-path resolution and returned None directly, bypassing the `sub()` interpolation path that would have resolved each expression. Downstream this surfaced as the literal string "None" reaching commands. Guard the fast path on `stripped.count("{{") == 1` so only genuine single-expression templates take the typed return; multi-expression templates fall through to `sub()` and interpolate correctly. Add regression tests for two expressions separated by a space and for adjacent expressions with no separator. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a workflow expression-evaluator edge case where templates containing multiple {{ ... }} blocks (and no non-whitespace surrounding text) could incorrectly take the single-expression fullmatch() fast path and return None, causing downstream commands to receive the literal string "None".
Changes:
- Adjust
evaluate_expression()to only use the typed single-expression fast path when the template is truly a single expression, otherwise fall back to interpolation. - Add regression tests covering multi-expression templates with whitespace-only separation and back-to-back expressions.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/workflows/expressions.py |
Guards the typed fullmatch() fast path so multi-expression templates interpolate correctly instead of collapsing to None. |
tests/test_workflows.py |
Adds regression tests to ensure multi-expression templates interpolate as expected. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Low
Comment on lines
+396
to
+400
| stripped = template.strip() | ||
| if stripped.count("{{") == 1: | ||
| match = _EXPR_PATTERN.fullmatch(stripped) | ||
| if match: | ||
| return _evaluate_simple_expression(match.group(1).strip(), namespace) |
mnriem
requested changes
Jun 29, 2026
mnriem
left a comment
mnriem
left a comment
Collaborator
There was a problem hiding this comment.
Please address Copilot feedback
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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.
What
evaluate_expressionreturnedNonefor any template containing two or more{{ }}blocks with no surrounding literal text, e.g."{{ context.run_id }} {{ inputs.issue }}". Downstream this surfaced as the literal string"None"reaching commands (the reporter hit "no issue was provided" despiteissue: "23"being in the run inputs).Fixes #3208.
Root cause
The single-expression fast path used
_EXPR_PATTERN.fullmatch(), butfullmatchdefeats the pattern's non-greedy(.+?)body. For"{{ a }} {{ b }}"it still matches, expanding the body to capture everything between the first{{and the last}}— i.e."a }} {{ b". That garbage body fails dot-path resolution and returnsNonedirectly, bypassing thesub()interpolation path that would have resolved each expression separately.fullmatch{{ inputs.issue }}inputs.issuebash x "{{ inputs.issue }}"sub(){{ a }} {{ b }}"a }} {{ b"How
Guard the fast path on
stripped.count("{{") == 1so only genuine single-expression templates take the typed-return path; multi-expression templates fall through to the existingsub()interpolation, which is already correct. Single-expression type preservation is unchanged.Tests
Added two regression tests to
TestExpressions:test_multi_expression_no_surrounding_text—"{{ context.run_id }} {{ inputs.issue }}"→"47c5eb4b 23"test_multi_expression_adjacent_no_separator—"{{ inputs.a }}{{ inputs.b }}"→"foobar"Verified both fail on
main(returnNone) and pass with the fix. All 31TestExpressionstests pass;ruffclean. (The 5 symlink-test failures on my local Windows run pre-exist onmain— a symlink-privilege limitation unrelated to this change; they pass on Linux CI.)🤖 Generated with Claude Code