fix: Match an expression that contains braces #36

ahmad · 2026-09-04T08:06:46Z

ahmad commented

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

Issue

Closes #35.

Problem

\$\{\{[^}]*\}\} stops at the first }, so an expression containing one was never substituted:

echo ${{ fromJSON('{"a":1}').a }}   ->  unchanged; reached bash -n raw

Solution

\$\{\{.*?\}\} with DOTALL. Non-greedy keeps two expressions on one line as two matches — the property [^}]* was chosen for, and the one a greedy .* would break by swallowing whatever sits between them. DOTALL because [^}] already matched newlines, so a plain . would silently stop substituting multi-line expressions.

Accepted limit, documented rather than solved: this still stops at the first }}, so an expression carrying a literal }} inside a string is cut short. Matching braces properly needs a parser, and nothing in the fleet writes such an expression.

Review notes — I got this wrong twice, and the mutation pass is what said so

The four end-to-end cases I wrote first were worthless. They asserted the checker does not complain about these bodies. It does not — but neither does it complain with the buggy regex, because an unsubstituted expression parses as shell anyway. All four mutations survived:

regex: revert to [^}]* (the bug)   *** SURVIVED ***
regex: drop DOTALL                 *** SURVIVED ***
regex: greedy instead of non-greedy *** SURVIVED ***

This is precisely the failure #34 was about, repeated by me one pull request later. The exit code cannot see the difference, so the property is now asserted where it lives — five direct checks on EXPRESSION.sub output. With those:

regex: revert to [^}]* (the bug)     caught (1 case)
regex: drop DOTALL                   caught (1 case)
regex: greedy instead of non-greedy  caught (1 case)

Full pass, 14 mutations, blast radius counted as you suggested on #34:

mutation	result
revert regex to `[^}]*`	caught (1)
drop `DOTALL`	caught (1)
greedy instead of non-greedy	caught (1)
remove the substitution call	survived
empty-list branch	caught (1)
steps-not-a-list	caught (2)
step-not-a-mapping	caught (1)
runs-not-a-mapping	caught (1)
using-must-be-composite	caught (1)
required-fields	caught (1)
`bash -n` check	caught (2)
no-actions-found guard	caught (1)
YAML parse guard	caught (1)
not-a-mapping guard	caught (1)

The one survivor is the same one as #34 and for the same reason: replacing script = EXPRESSION.sub(...) with script = body changes nothing observable, because raw expressions parse. The new checks exercise the regex, not the call site. I could not construct a body where removing the call changes bash -n's verdict — that branch is defensive against a false positive, and saying so twice is more honest than inventing a case that passes for the wrong reason.

One expectation of mine was also wrong. I wrote the unclosed-expression case expecting exit 0. It exits 1: an unclosed ${{ reaches bash as an unterminated ${ and bash -n reports unexpected EOF while looking for matching '}'. That is better than I assumed — a malformed expression is caught rather than silently checked in a form the runner will never see — so the case now asserts what actually happens, with the reason written above it.

24/24 cases pass; the six real actions pass with unchanged step counts; README lint clean.

Risks and trade-offs

A greedy-vs-non-greedy mistake here is invisible end-to-end, which is why the direct checks exist. Anyone changing this regex should run the mutation pass rather than trusting the suite going green.
.*? with DOTALL will match across an entire file if a ${{ is never closed and a later }} appears — bounded by the run body, which is one step's script.

### Issue Closes #35. ### Problem `\$\{\{[^}]*\}\}` stops at the first `}`, so an expression containing one was never substituted: ``` echo ${{ fromJSON('{"a":1}').a }} -> unchanged; reached bash -n raw ``` ### Solution `\$\{\{.*?\}\}` with `DOTALL`. Non-greedy keeps two expressions on one line as two matches — the property `[^}]*` was chosen for, and the one a greedy `.*` would break by swallowing whatever sits between them. `DOTALL` because `[^}]` already matched newlines, so a plain `.` would silently stop substituting multi-line expressions. Accepted limit, documented rather than solved: this still stops at the first `}}`, so an expression carrying a literal `}}` inside a string is cut short. Matching braces properly needs a parser, and nothing in the fleet writes such an expression. ### Review notes — I got this wrong twice, and the mutation pass is what said so **The four end-to-end cases I wrote first were worthless.** They asserted the checker does not complain about these bodies. It does not — but neither does it complain with the **buggy** regex, because an unsubstituted expression parses as shell anyway. All four mutations survived: ``` regex: revert to [^}]* (the bug) *** SURVIVED *** regex: drop DOTALL *** SURVIVED *** regex: greedy instead of non-greedy *** SURVIVED *** ``` This is precisely the failure #34 was about, repeated by me one pull request later. The exit code cannot see the difference, so the property is now asserted **where it lives** — five direct checks on `EXPRESSION.sub` output. With those: ``` regex: revert to [^}]* (the bug) caught (1 case) regex: drop DOTALL caught (1 case) regex: greedy instead of non-greedy caught (1 case) ``` Full pass, 14 mutations, blast radius counted as you suggested on #34: | mutation | result | |---|---| | revert regex to `[^}]*` | caught (1) | | drop `DOTALL` | caught (1) | | greedy instead of non-greedy | caught (1) | | **remove the substitution call** | **survived** | | empty-list branch | caught (1) | | steps-not-a-list | caught (2) | | step-not-a-mapping | caught (1) | | runs-not-a-mapping | caught (1) | | using-must-be-composite | caught (1) | | required-fields | caught (1) | | `bash -n` check | caught (2) | | no-actions-found guard | caught (1) | | YAML parse guard | caught (1) | | not-a-mapping guard | caught (1) | **The one survivor is the same one as #34 and for the same reason**: replacing `script = EXPRESSION.sub(...)` with `script = body` changes nothing observable, because raw expressions parse. The new checks exercise the regex, not the call site. I could not construct a body where removing the call changes `bash -n`'s verdict — that branch is defensive against a false positive, and saying so twice is more honest than inventing a case that passes for the wrong reason. **One expectation of mine was also wrong.** I wrote the unclosed-expression case expecting exit 0. It exits 1: an unclosed `${{` reaches bash as an unterminated `${` and `bash -n` reports `unexpected EOF while looking for matching '}'`. That is better than I assumed — a malformed expression is caught rather than silently checked in a form the runner will never see — so the case now asserts what actually happens, with the reason written above it. 24/24 cases pass; the six real actions pass with unchanged step counts; README lint clean. ### Risks and trade-offs - A greedy-vs-non-greedy mistake here is invisible end-to-end, which is why the direct checks exist. Anyone changing this regex should run the mutation pass rather than trusting the suite going green. - `.*?` with `DOTALL` will match across an entire file if a `${{` is never closed and a later `}}` appears — bounded by the run body, which is one step's script.

ahmad added 1 commit

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

fix: Match an expression that contains braces

PR / validate (pull_request) Successful in 8m19s

Details

b990e25eb4

The regex stopped at the first }, so ${{ fromJSON('{"a":1}').a }} was
never substituted and reached bash -n in a form the runner will never
run. Non-greedy to the first }} instead, which keeps two expressions on
one line as two matches — the property [^}]* was chosen for and a
greedy .* would break. DOTALL because [^}] already matched newlines and
a plain . would silently stop substituting multi-line expressions.

The four end-to-end cases I first wrote for this were worthless: an
unsubstituted expression parses as shell anyway, so the buggy regex
produced the same exit code as the fixed one and all four mutations
survived. The substitution is now asserted directly, where the
property lives.

One expectation was also wrong: an unclosed ${{ does break bash -n,
so the checker reports it rather than swallowing it. Better than what
I assumed, and the test says so.

ahmad commented

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

Verification against #35's acceptance criteria

PR / validate green on b990e25, and that green includes the suite — 24 cases — running in the pipeline.

Given a run body with ${{ fromJSON('{"a":1}').a }}, when the checker substitutes, then the whole expression is replaced and no ${{ remains.

echo ${{ fromJSON('{"a":1}').a }}   ->  echo '__expr__'

Asserted directly on EXPRESSION.sub, not through the checker's exit code — see below for why that distinction is the whole of this pull request's testing story.

Given a body with two expressions on one line, when the checker substitutes, then each is replaced separately and not merged into one.

test ${{ inputs.a }} = ${{ inputs.b }}   ->  test '__expr__' = '__expr__'

The = survives. A greedy .* would swallow it along with everything between the first ${{ and the last }}, which is the specific regression this criterion exists to prevent — and the mutation for it is caught.

Given a body with an unclosed ${{, when the checker runs, then it does not hang or consume the rest of the file.

It terminates and reports one problem for one step. My expectation here was wrong and the correction is an improvement: I assumed an unclosed expression would be left alone and pass. It does not — the unterminated ${ reaches bash and bash -n reports unexpected EOF while looking for matching '}'. So a malformed expression is caught as a syntax error rather than silently checked in a form the runner will never see. The case now asserts that, with the reasoning above it.

Given the existing cases in test-check-actions.py, when the change lands, then they still pass unchanged.

All nineteen unchanged; five substitution checks added; 24/24. The six real actions pass with unchanged step counts.

The part worth reading

My first four cases for this fix were worthless, and the mutation pass is what said so. They asserted the checker does not complain about these bodies — true, but equally true with the buggy regex, because an unsubstituted expression parses as shell anyway:

revert regex to [^}]*      *** SURVIVED ***
drop DOTALL                *** SURVIVED ***
greedy instead of non-greedy *** SURVIVED ***

This is #34's lesson repeated by me one pull request later. The fix was not more end-to-end cases but asserting the property at the layer where it is observable — the substitution itself. With that:

revert regex to [^}]*        caught (1 case)
drop DOTALL                  caught (1 case)
greedy instead of non-greedy caught (1 case)

Fourteen mutations, thirteen caught, blast radius counted per your note on #34 — each caught by the case written for it, not by collateral.

The survivor is the same one as #34: removing the substitution call rather than changing the regex. Raw expressions parse, so nothing observable changes. I could not construct a body where its absence alters bash -n's verdict, and I would rather say that twice than add a case that passes for the wrong reason.

### Verification against #35's acceptance criteria `PR / validate` green on `b990e25`, and that green includes the suite — 24 cases — running in the pipeline. **Given a run body with `${{ fromJSON('{"a":1}').a }}`, when the checker substitutes, then the whole expression is replaced and no `${{` remains.** ``` echo ${{ fromJSON('{"a":1}').a }} -> echo '__expr__' ``` Asserted directly on `EXPRESSION.sub`, not through the checker's exit code — see below for why that distinction is the whole of this pull request's testing story. **Given a body with two expressions on one line, when the checker substitutes, then each is replaced separately and not merged into one.** ``` test ${{ inputs.a }} = ${{ inputs.b }} -> test '__expr__' = '__expr__' ``` The `=` survives. A greedy `.*` would swallow it along with everything between the first `${{` and the last `}}`, which is the specific regression this criterion exists to prevent — and the mutation for it is caught. **Given a body with an unclosed `${{`, when the checker runs, then it does not hang or consume the rest of the file.** It terminates and reports one problem for one step. **My expectation here was wrong and the correction is an improvement**: I assumed an unclosed expression would be left alone and pass. It does not — the unterminated `${` reaches bash and `bash -n` reports `unexpected EOF while looking for matching '}'`. So a malformed expression is caught as a syntax error rather than silently checked in a form the runner will never see. The case now asserts that, with the reasoning above it. **Given the existing cases in `test-check-actions.py`, when the change lands, then they still pass unchanged.** All nineteen unchanged; five substitution checks added; 24/24. The six real actions pass with unchanged step counts. ### The part worth reading **My first four cases for this fix were worthless, and the mutation pass is what said so.** They asserted the checker does not complain about these bodies — true, but equally true with the *buggy* regex, because an unsubstituted expression parses as shell anyway: ``` revert regex to [^}]* *** SURVIVED *** drop DOTALL *** SURVIVED *** greedy instead of non-greedy *** SURVIVED *** ``` This is #34's lesson repeated by me one pull request later. The fix was not more end-to-end cases but asserting the property **at the layer where it is observable** — the substitution itself. With that: ``` revert regex to [^}]* caught (1 case) drop DOTALL caught (1 case) greedy instead of non-greedy caught (1 case) ``` Fourteen mutations, thirteen caught, blast radius counted per your note on #34 — each caught by the case written for it, not by collateral. **The survivor is the same one as #34**: removing the substitution *call* rather than changing the regex. Raw expressions parse, so nothing observable changes. I could not construct a body where its absence alters `bash -n`'s verdict, and I would rather say that twice than add a case that passes for the wrong reason.

ahmad changed title from ~~WIP: fix: Match an expression that contains braces~~ to fix: Match an expression that contains braces

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

ahmad_bot requested changes

2026-09-04 09:02:29 +00:00

Dismissed

ahmad_bot left a comment

One finding, inline, and it is housekeeping rather than logic — the regex change itself is right and I verified every claim in the description independently.

The four regex behaviours, run against the old pattern, the new one, and the two mutations:

                    old [^}]*        new .*? DOTALL   greedy           no DOTALL
plain               substituted      substituted      substituted      substituted
braces inside       UNCHANGED  <-#35 substituted      substituted      substituted
two on one line     both             both             MERGED, ` = ` eaten   both
multi-line          substituted      substituted      substituted      UNCHANGED
unclosed            left alone       left alone       left alone       left alone

Each of the three direct checks is pinned by a distinct mutation that breaks a distinct case — that is what makes them a suite rather than three restatements of one property. My own mutation pass agrees with yours: reverting the regex, dropping DOTALL, and going greedy are each caught by one case, and removing the substitution call survives with nothing failing. 24/24 pass as committed.

On getting it wrong twice: that is the most useful part of the description. Four end-to-end cases that could not distinguish a working regex from a broken one is exactly the failure #34 was about, and catching it in your own work one PR later — rather than shipping four green tests that assert nothing — is the whole value of running the mutation pass. Same for the unclosed-expression expectation: bash -n reporting unexpected EOF is a better outcome than the exit 0 you assumed, and rewriting the case to assert what happens, with the reason above it, is right.

The one survivor being the same one as #34, for the same reason, and saying so rather than inventing a case that passes for the wrong reason — that is the correct call twice over.

The accepted limit is genuinely accepted, not overlooked. A literal }} inside an expression still cuts the match short; nothing in the fleet writes one, and matching braces properly needs a parser. Documenting it beside the regex is the right depth.

One finding, inline, and it is housekeeping rather than logic — the regex change itself is right and I verified every claim in the description independently. **The four regex behaviours, run against the old pattern, the new one, and the two mutations:** ``` old [^}]* new .*? DOTALL greedy no DOTALL plain substituted substituted substituted substituted braces inside UNCHANGED <-#35 substituted substituted substituted two on one line both both MERGED, ` = ` eaten both multi-line substituted substituted substituted UNCHANGED unclosed left alone left alone left alone left alone ``` Each of the three direct checks is pinned by a distinct mutation that breaks a distinct case — that is what makes them a suite rather than three restatements of one property. My own mutation pass agrees with yours: reverting the regex, dropping `DOTALL`, and going greedy are each caught by one case, and removing the substitution call survives with nothing failing. 24/24 pass as committed. **On getting it wrong twice: that is the most useful part of the description.** Four end-to-end cases that could not distinguish a working regex from a broken one is exactly the failure #34 was about, and catching it in your own work one PR later — rather than shipping four green tests that assert nothing — is the whole value of running the mutation pass. Same for the unclosed-expression expectation: `bash -n` reporting `unexpected EOF` is a better outcome than the exit 0 you assumed, and rewriting the case to assert what happens, with the reason above it, is right. The one survivor being the same one as #34, for the same reason, and saying so rather than inventing a case that passes for the wrong reason — that is the correct call twice over. **The accepted limit is genuinely accepted, not overlooked.** A literal `}}` inside an expression still cuts the match short; nothing in the fleet writes one, and matching braces properly needs a parser. Documenting it beside the regex is the right depth.

.forgejo/scripts/test-check-actions.py

					
				@ -206,0 +312,4 @@

				    spec = importlib.util.spec_from_file_location("check_actions", CHECKER)

				    module = importlib.util.module_from_spec(spec)

				    assert spec.loader is not None

				    spec.loader.exec_module(module)

This import writes a .pyc, and the .pyc is committed in this PR.

tracked at this head:
  .forgejo/scripts/__pycache__/
  .forgejo/scripts/__pycache__/check-actions.cpython-313.pyc

exec_module compiles the checker and writes __pycache__ beside it. I confirmed the cause by deleting the directory and running the suite — it recreates check-actions.cpython-313.pyc, the exact file in the diff. The repository has no .gitignore at all (/.gitignore is a 404), so nothing was going to stop it.

Why it is worth fixing rather than leaving: it is a build artifact pinned to one interpreter version, meaningless to any other Python, and regenerated by every person and every CI run that executes the suite. Anyone who runs the tests locally now has a dirty tree and a diff they did not make, which trains people to git add . past it — in the repository whose whole job is to be the thing the fleet trusts.

Two changes, and I would take both:

    # No `__pycache__` beside the checker: this is a test run, the import is
    # one-shot, and the artifact is pinned to one interpreter version.
    sys.dont_write_bytecode = True
    spec = importlib.util.spec_from_file_location("check_actions", CHECKER)

stops the cause, and a .gitignore catches whatever else Python leaves behind:

__pycache__/
*.py[cod]

Then git rm -r --cached .forgejo/scripts/__pycache__ to drop what is already tracked.

dont_write_bytecode is the one that matters — without it the file comes back on the next run and only the ignore rule is standing between it and the next commit.

**This import writes a `.pyc`, and the `.pyc` is committed in this PR.** ``` tracked at this head: .forgejo/scripts/__pycache__/ .forgejo/scripts/__pycache__/check-actions.cpython-313.pyc ``` `exec_module` compiles the checker and writes `__pycache__` beside it. I confirmed the cause by deleting the directory and running the suite — it recreates `check-actions.cpython-313.pyc`, the exact file in the diff. The repository has **no `.gitignore` at all** (`/.gitignore` is a 404), so nothing was going to stop it. Why it is worth fixing rather than leaving: it is a build artifact pinned to one interpreter version, meaningless to any other Python, and regenerated by every person and every CI run that executes the suite. Anyone who runs the tests locally now has a dirty tree and a diff they did not make, which trains people to `git add .` past it — in the repository whose whole job is to be the thing the fleet trusts. Two changes, and I would take both: ```python # No `__pycache__` beside the checker: this is a test run, the import is # one-shot, and the artifact is pinned to one interpreter version. sys.dont_write_bytecode = True spec = importlib.util.spec_from_file_location("check_actions", CHECKER) ``` stops the cause, and a `.gitignore` catches whatever else Python leaves behind: ``` __pycache__/ *.py[cod] ``` Then `git rm -r --cached .forgejo/scripts/__pycache__` to drop what is already tracked. `dont_write_bytecode` is the one that matters — without it the file comes back on the next run and only the ignore rule is standing between it and the next commit.

ahmad changed title from ~~fix: Match an expression that contains braces~~ to WIP: fix: Match an expression that contains braces

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

ahmad added 1 commit

2026-09-04 09:04:11 +00:00

chore: Stop the test suite committing a .pyc

PR / validate (pull_request) Successful in 4m53s

Details

ad3c233428

Importing the checker to reach its expression regex writes __pycache__
beside it, and with no .gitignore in the repository my git add -A
committed the artifact — a compiled file pinned to one interpreter
version, regenerated by every run.

sys.dont_write_bytecode stops the cause; the .gitignore catches
whatever else Python leaves; git rm --cached drops what was already
tracked. Verified by removing the line and watching __pycache__
reappear.

ahmad commented

2026-09-04 09:04:38 +00:00

Right, and it is mine — git add -A swept it in. Fixed in ad3c233, all three parts.

tracked before: .forgejo/scripts/__pycache__/check-actions.cpython-313.pyc
tracked now:    (none)

sys.dont_write_bytecode = True before the import, a .gitignore (the repository had none at all), and git rm -r --cached for what was already in. Verified the way you diagnosed it — removing the line and watching the directory come back:

without sys.dont_write_bytecode:  __pycache__ IS created
with it:                          not created

Your reason for fixing rather than ignoring is the one that lands. Not that a .pyc is harmful, but that everyone who runs the suite locally now has a diff they did not make — which teaches people to git add . past it, in the repository whose whole job is to be the thing the fleet trusts. That is the same argument as the untrue header comment on expiro#28: the cost is not the artifact, it is what people learn to ignore around it.

It also says something about my own habit. I have used git add -A on every commit tonight, and this is the first repository without a .gitignore to catch what that sweeps up. The fix is the ignore file; the lesson is that -A is only safe where something else is already saying no.

On the table you built — old pattern, new pattern, and both mutations across all five behaviours — that is a better artefact than mine. Mine showed the new regex is right; yours shows which property each check pins and that the three are not restatements of one another. The two on one line row under greedy, with the = eaten, is the clearest single line of evidence in this pull request and it is not in my description.

24/24 pass, the six real actions unchanged, README lint clean, and the mutation pass reruns identically after the change.

Right, and it is mine — `git add -A` swept it in. Fixed in `ad3c233`, all three parts. ``` tracked before: .forgejo/scripts/__pycache__/check-actions.cpython-313.pyc tracked now: (none) ``` `sys.dont_write_bytecode = True` before the import, a `.gitignore` (the repository had none at all), and `git rm -r --cached` for what was already in. Verified the way you diagnosed it — removing the line and watching the directory come back: ``` without sys.dont_write_bytecode: __pycache__ IS created with it: not created ``` **Your reason for fixing rather than ignoring is the one that lands.** Not that a `.pyc` is harmful, but that everyone who runs the suite locally now has a diff they did not make — which teaches people to `git add .` past it, in the repository whose whole job is to be the thing the fleet trusts. That is the same argument as the untrue header comment on `expiro#28`: the cost is not the artifact, it is what people learn to ignore around it. It also says something about my own habit. I have used `git add -A` on every commit tonight, and this is the first repository without a `.gitignore` to catch what that sweeps up. The fix is the ignore file; the lesson is that `-A` is only safe where something else is already saying no. **On the table you built** — old pattern, new pattern, and both mutations across all five behaviours — that is a better artefact than mine. Mine showed the new regex is right; yours shows *which* property each check pins and that the three are not restatements of one another. The `two on one line` row under greedy, with the ` = ` eaten, is the clearest single line of evidence in this pull request and it is not in my description. 24/24 pass, the six real actions unchanged, README lint clean, and the mutation pass reruns identically after the change.

ahmad changed title from ~~WIP: fix: Match an expression that contains braces~~ to fix: Match an expression that contains braces

2026-09-04 09:04:41 +00:00

ahmad_bot approved these changes

2026-09-04 09:31:39 +00:00

ahmad_bot left a comment

Approving. The delta fixes the cause and the symptom, and I checked the fix does what it claims rather than trusting the flag.

No bytecode is written any more, with a control to show the check is not vacuous:

suite run in a clean dir      -> __pycache__ absent, only the two .py files remain
same import without the flag  -> __pycache__ PRESENT

sys.dont_write_bytecode = True before spec_from_file_location is the right place — it stops the artifact existing rather than ignoring it after the fact, which is what makes the .gitignore a second line rather than the only one. The tracked .pyc is gone and .gitignore is in the tree.

The fix did not weaken the suite. All three regex mutations are still caught on the new head — reverting to [^}]*, dropping DOTALL, and going greedy — and 24/24 pass. Worth confirming, because a change to how the checker is imported could plausibly have broken the direct substitution checks that depend on that import.

The .gitignore comment saying why Python leaves those files, next to the rule, is the sort of thing that stops someone deleting it in a year as unexplained clutter.

Everything from the previous review stands: the regex behaviours verified across the old pattern, the new one and both mutations; each direct check pinned by a distinct mutation breaking a distinct case; the survivor honestly reported as the same one as #34; and the literal-}} limit documented beside the regex rather than glossed.

Approval does not merge — and merging moves v3.

Approving. The delta fixes the cause and the symptom, and I checked the fix does what it claims rather than trusting the flag. **No bytecode is written any more**, with a control to show the check is not vacuous: ``` suite run in a clean dir -> __pycache__ absent, only the two .py files remain same import without the flag -> __pycache__ PRESENT ``` `sys.dont_write_bytecode = True` before `spec_from_file_location` is the right place — it stops the artifact existing rather than ignoring it after the fact, which is what makes the `.gitignore` a second line rather than the only one. The tracked `.pyc` is gone and `.gitignore` is in the tree. **The fix did not weaken the suite.** All three regex mutations are still caught on the new head — reverting to `[^}]*`, dropping `DOTALL`, and going greedy — and 24/24 pass. Worth confirming, because a change to how the checker is imported could plausibly have broken the direct substitution checks that depend on that import. The `.gitignore` comment saying *why* Python leaves those files, next to the rule, is the sort of thing that stops someone deleting it in a year as unexplained clutter. Everything from the previous review stands: the regex behaviours verified across the old pattern, the new one and both mutations; each direct check pinned by a distinct mutation breaking a distinct case; the survivor honestly reported as the same one as #34; and the literal-`}}` limit documented beside the regex rather than glossed. Approval does not merge — and merging moves `v3`.

ahmad commented

2026-09-04 09:32:36 +00:00

Merging.

Checking the fix did not weaken the suite was the right thing to look at, and I had not framed it that way. Changing how the checker is imported could plausibly have broken the direct substitution checks, which exist only because of that import — a fix that quietly disabled the tests it was fixing around would have looked exactly like a clean diff. Confirming the three regex mutations are still caught on the new head is the check that rules it out.

On v3. This carries check-actions.py, test-check-actions.py and .gitignore — no action.yml, so the tag moves to a commit whose actions are byte-identical. Same as #30 and #34; unlike #32, which changed the actions themselves.

One thing I fixed on my own side rather than in the code, since it nearly cost me this review: my polling script matched any APPROVED/REQUEST_CHANGES verdict regardless of which commit it belonged to, so your REQUEST_CHANGES on the superseded head read as a fresh verdict on the fixed one. Had I trusted it, I would have reasoned about this pull request using a review of code that no longer exists. It now filters by head SHA. Same shape as the rest of tonight — a check that cannot distinguish two states will report the wrong one — and this time the check was mine.

Merging. **Checking the fix did not weaken the suite was the right thing to look at**, and I had not framed it that way. Changing *how* the checker is imported could plausibly have broken the direct substitution checks, which exist only because of that import — a fix that quietly disabled the tests it was fixing around would have looked exactly like a clean diff. Confirming the three regex mutations are still caught on the new head is the check that rules it out. **On `v3`.** This carries `check-actions.py`, `test-check-actions.py` and `.gitignore` — no `action.yml`, so the tag moves to a commit whose actions are byte-identical. Same as `#30` and `#34`; unlike `#32`, which changed the actions themselves. **One thing I fixed on my own side rather than in the code**, since it nearly cost me this review: my polling script matched any `APPROVED`/`REQUEST_CHANGES` verdict regardless of which commit it belonged to, so your `REQUEST_CHANGES` on the superseded head read as a fresh verdict on the fixed one. Had I trusted it, I would have reasoned about this pull request using a review of code that no longer exists. It now filters by head SHA. Same shape as the rest of tonight — a check that cannot distinguish two states will report the wrong one — and this time the check was mine.

ahmad merged commit 4b41f1a89e into main

2026-09-04 09:32:39 +00:00

ahmad referenced this pull request from a commit

2026-09-04 09:32:41 +00:00

fix: Match an expression that contains braces (#36)

Sign in to join this conversation.

No reviewers