fix(release): repair an unterminated shell string, and guard the class - #57
Merged
Conversation
release.yml's webconsole job carried
python -m pip install --quiet "packaging==$PKG_PIN\"
A stray backslash before the closing quote left the shell string
unterminated, so that job would have died the first time anyone cut a
webconsole tag. I introduced it in #48 while fixing a DIFFERENT escape bug:
a sed backreference written into a non-raw Python string became chr(1), I
switched to raw strings, and a raw string then preserved the \" I did not
want. Two escape failures from opposite directions in one edit.
Nothing could have caught it, which is the more interesting half:
- the YAML parsed, because a broken shell string is still a valid YAML
scalar;
- the pin guard passed, because it greps for pin syntax and the line does
contain packaging==;
- no CI leg executes it -- the step is tag-only and webconsole-gated, and
no pull request runs a tag-only release job.
So the first execution would have been a release. A one-line fix leaves that
hole open for the next edit, so this adds tests/test_workflow_shell_syntax.py:
parse every workflow, extract every shell run: block, and ask bash -n whether
it is syntactically valid. It resolves the shell the way Actions does (step >
job defaults > workflow defaults), skips pwsh blocks and Windows runners with
an unspecified shell, and substitutes ${{ ... }} first since Actions templating
is not shell.
126 blocks across 12 workflows check clean. Two liveness tests sit in front of
the check because a glob or extractor that silently found nothing would make it
vacuous -- it asserts the block count instead of trusting a green.
Proven to kill the real bug, mutation verified as APPLIED first: reintroducing
the backslash exits 1, restoring exits 0. An unapplied mutation reads as a pass,
so the injection is byte-checked before the result is believed.
One implementation note worth keeping: bash -n must take a FILE, not stdin. A
script containing a heredoc makes bash read the heredoc body from the same
stream it is reading the script from, and it blocks for a terminator that never
arrives -- a 30s timeout, not a syntax error. The timeout is caught and named
per block rather than failing the run blind.
Scope stated in the docstring: this checks syntax, not behaviour. It cannot
catch a command that parses and then does the wrong thing.
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
1 participant
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.
release.yml's webconsole job carriedpip install --quiet "packaging==$PKG_PIN\". The stray backslash left the shell string unterminated, so that job would have died the first time anyone cut awebconsole-*tag.I introduced it in #48, while fixing a different escape bug: a sed backreference in a non-raw Python string became
chr(1), I switched to raw strings, and the raw string then preserved the\"I didn't want. Two escape failures from opposite directions in one edit.Nothing could have caught it
packaging==.webconsole-*-gated, and no pull request runs a tag-only release job.So the first execution would have been a release. A one-line fix leaves the hole open for the next edit, so this adds
tests/test_workflow_shell_syntax.py: parse every workflow, extract every shellrun:block, and askbash -nwhether it is syntactically valid. It resolves the shell the way Actions does (step > job defaults > workflow defaults), skipspwshand Windows runners with an unspecified shell, and substitutes${{ ... }}first since Actions templating isn't shell.126 blocks across 12 workflows check clean. Two liveness tests sit in front of the check, asserting the block count — a glob or extractor that silently found nothing would make the whole thing vacuous.
Proven to kill the real bug
Mutation verified as applied before the result was believed: reintroducing the backslash exits 1, restoring exits 0. An unapplied mutation reads as a pass, so the injection is byte-checked first.
One implementation note worth keeping:
bash -nmust take a FILE, not stdin. A script containing a heredoc makes bash read the heredoc body from the same stream it's reading the script from, and it blocks for a terminator that never arrives — a 30 s timeout, not a syntax error. That timeout is caught and named per block rather than failing blind.Scope, stated honestly
This checks syntax, not behaviour. It cannot catch a command that parses and then does the wrong thing. The failure it exists to prevent was purely syntactic.
Verification: ruff + format clean · mypy clean · the new module 3 passed · the four
.github-reading suites 27 passed.