Repository navigation
fix(ci): widen the marker check to the filters it guards - #279
Conversation
![sasjs-dev[bot]](https://avatars.githubusercontent.com/u/67858352?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the full base...head diff (one file, .git-hooks/commit-msg) against semantic-release 25.0.9's preprocess filter and GitHub's documented skip markers. The widening is correct in coverage: the new pattern at :40 catches both whitespace-tolerant release forms, all four GitHub skip strings, and the case-insensitive variants the old pattern let through, while the chore(release) exemption at :17-22 still precedes it. Two findings, one on the change and one on the PR itself:
-
.git-hooks/commit-msg:40 - the pattern uses \s inside grep -E. GNU grep accepts it; BSD grep, the default on macOS where contributors run this hook, does not treat \s as whitespace, so on those machines the widened pattern silently stops matching every real marker and the guard fails open - the exact failure this PR exists to close. Replace each \s with [[:space:]] (POSIX ERE, both greps). The identical line also ships in sasjs/adapter#919, so the fix belongs in both.
-
this PR's head commit e26b30f has produced zero workflow runs - no check runs, no check suites, nothing queued - while its parent on main and the neighbouring PRs all ran. The cause is in the commit itself: its body carries the literal strings "[skip ci]", "[no ci]", "[skip actions]" and "[actions skip]" (the verification paragraph listing the shapes), and GitHub's skip instruction matches the marker anywhere in the commit message, not only in the subject. The very hook this PR widens exists because of that mechanism, and its own documentation has tripped it: the run-tests and windows workflows that would have gated this PR never started, so the PR shows green-by-absence rather than green. Amend the commit (or push a new one) with the markers obfuscated - for example bracket-joined like "[skip" + " ci]" or spelled out in words - the same care the hook's comments take when naming them, and so the PR gets a real CI verdict before merge. Note the hook itself checks only the subject, so the reworded body stays hook-clean.
2 findings above for review.
The subject-only check caught the release marker and the ci marker, but missed their upper-case forms, the reversed word order, and the four GitHub forms - ci-skip, no-ci, skip-actions and actions-skip - which suppress a run just as completely. Both upstream filters are case-insensitive and accept any run of whitespace, so the pattern now mirrors them. This matters because both act on the WHOLE squashed commit rather than on the one commit carrying the marker: the release tooling drops any commit whose message matches, and GitHub skips the push-triggered workflow outright. Either one silently suppresses the release for the entire PR. Not hypothetical - it happened here. #274 carried two chore(ci) subjects with the release marker, so the squashed commit on main was filtered out whole, the crypto fix merged, and no version was published. The pattern uses a POSIX character class rather than backslash-s. Backslash-s is a GNU extension that BSD grep does not honour, so on macOS - where contributors run this hook - the pattern would stop matching every real marker and the guard would fail open, which is the failure this commit exists to close. Verified against 20 subject shapes: every conventional type with and without a scope passes, the release tooling's own commit is exempted, both release forms and all four GitHub forms are rejected in upper and lower case and with multiple spaces, a bare non-conventional subject is rejected, and a marker appearing only in the body still passes because the body never reaches the squash.
…udit The audit caught a new advisory, which is what it is for: GHSA-rj75-hqrm-r3gf, moderate, quadratic complexity in flat selector parsing. It cannot be fixed from here. The vulnerable copy is npm's own bundled dependency - release/node_modules/npm/node_modules/postcss-selector-parser - so no resolver change reaches it, and 11.21.0 is the last npm 11.x while @semantic-release/npm requires ^11. The eventual fix is that plugin moving to npm 12. It is also unreachable in this usage: the parser reads this repo's own release config, not the commits being matched. Named with that reasoning rather than silenced, so the audit still fails on anything new. 13 exempt, exit 0.
sas-language is the only one of the five repos that lints release/, and its prettier config sets printWidth 100, so the exemption block needed reformatting to pass that gate. The file stays byte-identical across all five.
e26b30f to
6365758
Compare
|
Both findings fixed, and thank you - the second one is the root cause of a CI anomaly I had been chasing for hours. Finding 1 - Finding 2 - the body carried the markers. This is the cause of something I had recorded as an unexplained anomaly: utils#277 and adapter#919 produced no The commit is amended. The body now names the markers in words rather than quoting them - the release marker, ci-skip, no-ci, skip-actions, actions-skip - and backslash-s is described rather than written. Confirmed clean: The hook itself checks only the subject, so the reworded body stays hook-clean. Also folded in: utils#280 (the toolchain-audit exemption for the new postcss-selector-parser advisory) now lives on this branch rather than as a separate PR, so this is one utils change to review. Its two commits are |
|
🎉 This PR is included in version 3.7.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
What
The marker check was narrower than the filters it guards.
semantic-release's skip filter is case-insensitive and accepts any run of whitespace (
/\[skip\s+release]|\[release\s+skip]/i,lib/definitions/plugins.js), so[SKIP RELEASE]and[release skip]passed this hook and still suppressed the release. GitHub also skips the push or pull_request run entirely on[ci skip],[no ci],[skip actions]and[actions skip], none of which were covered - only[skip ci]was.The pattern now mirrors both, and semantic-release's own
chore(release): X [skip ci]commit stays exempt (it is pushed straight to the branch, never squash-merged).Why it is here and not on #277
Found in review on sasjs/adapter#919, which carries a byte-identical copy of this hook. The two are kept identical, so the fix belongs in both. #277 and #278 are already merged, hence this PR.
Verification
12 subject shapes, all as expected:
[skip release],[SKIP RELEASE],[release skip],[skip release],[ci skip],[no ci],[skip actions],[actions skip]chore(release): 3.7.1 [skip ci],chore(deps): refresh the lockfile,fix(utils): ..., and afix:whose body mentions the markers (the body never reaches the squash commit)