amtronics/platform-actions

Watch

Fork 0

chore: The expression regex misses an expression containing braces #35

New issue

Closed

opened 2026-09-04 06:36:48 +00:00 by ahmad · 1 comment

ahmad commented

2026-09-04 06:36:48 +00:00

Owner

Context

check-actions.py substitutes ${{ ... }} before running bash -n, using:

EXPRESSION = re.compile(r"\$\{\{[^}]*\}\}")

[^}]* stops at the first }, so an expression containing one is not matched and reaches bash -n unsubstituted:

echo ${{ inputs.x }}                  ->  echo '__expr__'                     substituted
echo ${{ fromJSON('{"a":1}').a }}     ->  echo ${{ fromJSON('{"a":1}').a }}   unchanged
echo ${{ inputs.a }} ${{ inputs.b }}  ->  echo '__expr__' '__expr__'          substituted

Found while mutation-testing the checker in #34: deleting the substitution entirely did not fail the suite, and probing why turned up this.

Latent. No action in this repository uses an expression containing a brace, and an unsubstituted expression does not reliably make bash -n fail — so today the consequence is a body checked in a form the runner will never see, rather than a false result. It becomes real if such an expression is added and happens to be a shape bash rejects.

Scope

In: matching an expression that contains braces, without matching past the end of one.
Out: validating the expression's own syntax; anything else in the checker.

Acceptance criteria

Given a run body with ${{ fromJSON('{"a":1}').a }}, when the checker substitutes, then the whole expression is replaced and no ${{ remains.
Given a body with two expressions on one line, when the checker substitutes, then each is replaced separately and not merged into one.
Given a body with an unclosed ${{, when the checker runs, then it does not hang or consume the rest of the file.
Given the existing cases in test-check-actions.py, when the change lands, then they still pass unchanged.

Notes

[^}]* was presumably chosen to stay non-greedy across multiple expressions on one line — the third row above is the case a naive .* would break by merging two expressions into one. Whatever replaces it has to keep that, which is why it is a criterion rather than an afterthought.

### Context `check-actions.py` substitutes `${{ ... }}` before running `bash -n`, using: ```python EXPRESSION = re.compile(r"\$\{\{[^}]*\}\}") ``` `[^}]*` stops at the first `}`, so an expression containing one is not matched and reaches `bash -n` unsubstituted: ``` echo ${{ inputs.x }} -> echo '__expr__' substituted echo ${{ fromJSON('{"a":1}').a }} -> echo ${{ fromJSON('{"a":1}').a }} unchanged echo ${{ inputs.a }} ${{ inputs.b }} -> echo '__expr__' '__expr__' substituted ``` Found while mutation-testing the checker in #34: deleting the substitution entirely did not fail the suite, and probing why turned up this. **Latent.** No action in this repository uses an expression containing a brace, and an unsubstituted expression does not reliably make `bash -n` fail — so today the consequence is a body checked in a form the runner will never see, rather than a false result. It becomes real if such an expression is added and happens to be a shape bash rejects. ### Scope - In: matching an expression that contains braces, without matching past the end of one. - Out: validating the expression's own syntax; anything else in the checker. ### Acceptance criteria - [ ] Given a run body with `${{ fromJSON('{"a":1}').a }}`, when the checker substitutes, then the whole expression is replaced and no `${{` remains. - [ ] Given a body with two expressions on one line, when the checker substitutes, then each is replaced separately and not merged into one. - [ ] Given a body with an unclosed `${{`, when the checker runs, then it does not hang or consume the rest of the file. - [ ] Given the existing cases in `test-check-actions.py`, when the change lands, then they still pass unchanged. ### Notes `[^}]*` was presumably chosen to stay non-greedy across multiple expressions on one line — the third row above is the case a naive `.*` would break by merging two expressions into one. Whatever replaces it has to keep that, which is why it is a criterion rather than an afterthought.

ahmad added the

labels

2026-09-04 06:36:48 +00:00

ahmad self-assigned this

2026-09-04 08:03:00 +00:00

ahmad added

state/in-progress

and removed

state/ready

labels

2026-09-04 08:03:01 +00:00

ahmad commented

2026-09-04 08:03:03 +00:00

Author

Owner

Picking this up — expiro has had no runner contact with Docker Hub for six hours, so this repository is still the only place work finishes.

The change is one line. \$\{\{[^}]*\}\} becomes \$\{\{.*?\}\} with DOTALL. Non-greedy to the first }} keeps the property [^}]* was presumably chosen for — two expressions on one line stay two matches rather than merging into one — while no longer stopping at a brace inside the expression.

Why DOTALL is not incidental. [^}] already matched newlines, so a multi-line expression was being substituted; . without the flag would not, and dropping it would be a silent regression the existing cases do not cover. That is the one way this change could quietly make things worse, so it gets a test of its own.

The limit I am accepting rather than solving. Non-greedy still stops at the first }}, so an expression containing a literal }} inside a string — ${{ fromJSON('{{"a":1}}') }} — is cut short. Matching braces properly needs a parser rather than a regex, and no action in the fleet or in GitHub's own documentation writes one; the honest answer is to document the boundary, not to build the parser.

Four cases go into test-check-actions.py — braces inside, two on a line, multi-line, unclosed — and I will run the mutation pass on the new branch as well as the existing eleven, since a regex change that the suite cannot detect is exactly the thing #34 was about.

Picking this up — `expiro` has had no runner contact with Docker Hub for six hours, so this repository is still the only place work finishes. **The change is one line.** `\$\{\{[^}]*\}\}` becomes `\$\{\{.*?\}\}` with `DOTALL`. Non-greedy to the first `}}` keeps the property `[^}]*` was presumably chosen for — two expressions on one line stay two matches rather than merging into one — while no longer stopping at a brace inside the expression. **Why `DOTALL` is not incidental.** `[^}]` already matched newlines, so a multi-line expression was being substituted; `.` without the flag would not, and dropping it would be a silent regression the existing cases do not cover. That is the one way this change could quietly make things worse, so it gets a test of its own. **The limit I am accepting rather than solving.** Non-greedy still stops at the *first* `}}`, so an expression containing a literal `}}` inside a string — `${{ fromJSON('{{"a":1}}') }}` — is cut short. Matching braces properly needs a parser rather than a regex, and no action in the fleet or in GitHub's own documentation writes one; the honest answer is to document the boundary, not to build the parser. Four cases go into `test-check-actions.py` — braces inside, two on a line, multi-line, unclosed — and I will run the mutation pass on the new branch as well as the existing eleven, since a regex change that the suite cannot detect is exactly the thing #34 was about.

ahmad referenced this issue from a pull request that will close it,

2026-09-04 08:06:46 +00:00

fix: Match an expression that contains braces #36

ahmad added

state/in-review

and removed

state/in-progress

labels

2026-09-04 08:07:09 +00:00

ahmad referenced this issue

2026-09-04 08:32:51 +00:00

fix: Match an expression that contains braces #36