ci: check that the squash subject carries a type cliff.toml keeps - #1069
Open
fujibee wants to merge 4 commits into
Open
ci: check that the squash subject carries a type cliff.toml keeps#1069fujibee wants to merge 4 commits into
fujibee wants to merge 4 commits into
Conversation
Every PR here lands as a squash whose subject is, per the repository setting COMMIT_OR_PR_TITLE, the single commit's subject for a one-commit PR and the PR title otherwise (measured on the last 30 merges into main: 3 of 3 and 5 of 5 where the two differed). A subject without a prefix cliff.toml classifies is silently dropped from the release notes; that happened to 1.1.7's headline feature and to four landings on the 1.3.0 integration branch, plus #1043 on main. The checker takes the title, the commit count and the head, judges the subject that will actually land, and derives the accepted types from cliff.toml's commit_parsers at run time rather than retyping them -- `spawn:` has the shape of a type and is not one. It exits 2, never 0, when it has nothing to check: no inputs, a bad commit count, a head it cannot read (the title is not a fallback), or a cliff.toml whose derived set lacks feat/fix. That last case fired during development, when a BSD-sed-incompatible pattern derived an empty set. The workflow runs on pull_request with `edited` among the event types, because a title is fixed by editing it and the default types would never re-check the fix. It checks out the PR head at depth 1 so a one-commit PR's subject is readable from git. Controls in tests/test_check_squash_subject.bats: the landed-wrong subjects are red, good ones green, the derivation agrees with an independent scrape of cliff.toml, the one-commit/multi-commit selection is pinned in both directions, and each zero-target case is exit 2. Calibrated by mutation: an empty derivation reddens 12 of 15, ignoring --repo reddens the two one-commit tests, and falling back to the title on an unreadable head reddens exactly the control written for it.
…shape Review found the first version wrong in both directions: it collected every anchored pattern in cliff.toml as an accepted type, so `release:` and `chore(ci):` -- which hit skip rules and are dropped -- came back green, while the legacy group rules for non-Conventional subjects (`native windows`, `Role-to-session affinity`, ...) -- which git-cliff keeps -- came back red. The shape of the subject was a proxy for the harm, and the proxy and the harm disagree exactly there. The checker now decides keep or drop the way git-cliff does, measured with git-cliff 2.10.1 against this cliff.toml on a fixture repository: the commit_parsers are tried in file order and the first match decides; a rule with `group` keeps, a rule with `skip = true` drops, the trailing `.*` drops the rest; matching is a regex search, so `^feat` also keeps `feature-flag: ...` and `Feat:` is dropped; and with `filter_unconventional` absent git-cliff's default drops every non-Conventional commit before the rules run (this cliff.toml sets it to false). The rules are read out of cliff.toml at run time; `--parsers` prints them as derived. One measured divergence is documented and pinned: unanchored rules also search the commit body, and the checker reads the subject line only. The test table carries git-cliff's own verdict for each subject and is re-measured against the real git-cliff wherever it is installed (skipped visibly elsewhere); the derived rules are compared with an independent scrape in order and in kind; first-match order and the filter_unconventional default each have a differential pair. Also fixed: an option given without its value made `shift 2` fail without shifting, and the argument loop spun on it forever; it is now exit 2.
…eaking protection Two corrections from review. First, the harm is not "absent from the notes" but "absent by accident": a subject a skip rule matches (`chore:`, `ci:`, `release:`) is left out because cliff.toml says so, and a check that reddened it would block a CI-only PR from landing -- this one included. So there are three answers now, and only the third is red: a keep rule, a skip rule written for the subject, or nothing but the catch-all. The catch-all is recognised as a skip rule that matches everything (any pattern matching the empty string), so landing on it is never mistaken for a decision. Second, git-cliff's `protect_breaking_commits` keeps a breaking commit past any skip rule and past the catch-all -- measured with git-cliff 2.10.1: `chore!:`, `release!:` and `wip!:` are all kept when it is true and dropped when it is false. The checker now reads that flag, `conventional_commits` and `filter_unconventional` from cliff.toml, uses git-cliff's measured default only when a key is absent (true / true / false), and prints the three with their source in every verdict, so a defaulted value is never silent. Breaking protection needs Conventional parsing (measured: conventional_commits=false switches it off), and the `!` in the subject is what the checker can see. A BREAKING CHANGE footer lives in the body, which the checker does not read: git-cliff keeps such a subject, the checker reports red. That blind spot is deliberately on the red side, and the header says so. Tests: the table gains the four breaking rows and re-measures against the real git-cliff; the catch-all versus an explicit skip rule, the protection flag, the defaults, conventional_commits=false and the footer blind spot each have a differential pair; the table greps use fixed strings.
The header said a subject that hits a skip rule and is breaking only in its body comes back red. Under the three answers it does not: a skip-rule match is green with or without a footer, and only the stated reason can differ from what git-cliff does. The one case the footer changes is a subject that matches nothing but the catch-all. Say that, and pin the skip-rule side in the test next to the catch-all side.
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.
A PR whose squash subject matches none of cliff.toml's rules lands, falls to
the catch-all, and is then silently absent from the release notes -- without
anyone having decided that. 1.1.7 lost a headline feature that way, and on the
1.3.0 integration branch it happened four more times:
and once more on main:
Reduce repeated ciphertext literals in pull apply(#1043).Each time the review looked at the change and nobody at the subject. This adds a
checker for the subject, with a workflow that runs it on every PR and re-runs it
when the title is edited.
What it decides
Three answers, and only the third is red:
The harm is not "absent from the notes"; it is "absent by accident". A check
that reddened every dropped subject would block a CI-only PR from landing
(this one included), and a check on the Conventional shape answers a
different question again: a Conventional
chore:is excluded and anon-Conventional
Add native Windows supportis kept by a legacy rule.The checker rejects the subject no rule was written for; it does not bind new
subjects to the Conventional shape, and the two are not mixed.
The model, measured
Everything about "matches" is measured against git-cliff 2.10.1 with this
cliff.toml on fixture repositories, and re-measured by the test suite
wherever git-cliff is installed:
Three
[git]keys change the answer. Each is read from cliff.toml; git-cliff'sown default (measured) is used only when the key is absent, and every verdict
prints the three with their source, so a defaulted value is never silent:
The catch-all is recognised as a skip rule that matches everything (any
pattern that matches the empty string), so it is never mistaken for a
decision about the subject. The rules and flags are read out of cliff.toml at
run time (
--parsersprints them as derived), so a change there is honouredby the next run.
Blind spot, on the red side. The checker reads the subject line only. A
BREAKING CHANGE:footer lives in the body, so a subject that matches onlythe catch-all and is breaking only in its body comes back red -- git-cliff
keeps it (measured), but that cannot be detected here. That is the only case
the footer changes: a subject a skip rule matches is green with or without a
footer, since none of the three answers depends on the body. The
body is not taken as input because at PR time the squash body is not yet
fixed, and judging something that can still change is how a "passed, then
dropped" happens. A false red is seen by a person, who fixes one line or
waives it; a false green is seen by nobody. If such a subject must pass, put
the
!in the subject, or a person decides. The header says all this, and atest pins it.
Which subject
The repository squashes every PR with
squash_merge_commit_title = COMMIT_OR_PR_TITLE. That setting is not "the PR title". Measured on the 30 mostrecent PRs merged into main, comparing the landed subject (PR-number suffix
stripped) with the title and with the first commit's subject:
So the script takes the title, the commit count and the head, and judges the one
that will land. #1043 is the case a title-only check gets wrong: one commit, and
the commit is what landed. The workflow checks out the PR head at depth 1 so the
commit's subject is readable from git.
What it returns when it has nothing to check
Exit 2, never 0, for: no subject and no inputs; a missing or non-numeric commit
count; an option without its value (this used to spin forever:
shift 2withone argument left fails without shifting); a one-commit PR whose head cannot be
read (the title is NOT used as a fallback -- that would be a green about a
subject never read); a cliff.toml that cannot be read or has no usable rules.
Every verdict names which subject it read, why, which rule decided, which of
the three answers it is, and the three flags with their source.
Controls (tests/test_check_squash_subject.bats, 24)
(lost),
chore(ci):/release:/ci:(Conventional, excluded on purpose),Add native Windows support/Role-to-session affinity:(non-Conventional,kept),
feature-flag:(kept by^feat),Feat:(lost), and the fourbreaking subjects
chore!:/release!:/chore(ci)!:/wip!:(kept) --read by the rule test AND re-measured against the real git-cliff wherever
it is installed, with the real
[git]section used verbatim under anid-printing template (git-cliff lists exactly the
keeprows); skippedvisibly where git-cliff is absent
versus an explicit skip rule; first-match order; protect_breaking_commits
true/false past a skip rule and past the catch-all; a flag absent versus
present, with the output saying which was defaulted;
conventional_commits=false switching protection off; filter_unconventional
absent
in order and in kind
the commit; the reverse is green; a multi-commit PR is judged by the title
loop with a bounded wait so a regression is a red, not a hang
editedCalibrated one change at a time, each reddening only its own controls:
counting the catch-all as a deliberate skip; counting a skip rule as a loss;
ignoring breaking protection; assuming the wrong default for
protect_breaking_commits; and each of the seven exit-2 guards turned into an
exit 0 on its own.
Not in this PR
Making the check required on main is a repository setting. The check does
not run on
push: there is no PR subject to read there.Local: the bats file 24/24 on bash 5.3 (git-cliff cross-check executed, not
skipped); the checker run by hand under /bin/bash 3.2;
check-enforced-assertionsat its baseline.