From 506d4b9a77e0a0e422daa6ccb35d5f6010d1764e Mon Sep 17 00:00:00 2001 From: fujibee Date: Mon, 7 Sep 2026 02:01:03 -0700 Subject: [PATCH 1/4] ci: check that the squash subject carries a type cliff.toml keeps 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. --- .github/scripts/check-squash-subject.sh | 141 ++++++++++++++++ .github/workflows/squash-subject.yml | 51 ++++++ tests/test_check_squash_subject.bats | 216 ++++++++++++++++++++++++ 3 files changed, 408 insertions(+) create mode 100755 .github/scripts/check-squash-subject.sh create mode 100644 .github/workflows/squash-subject.yml create mode 100644 tests/test_check_squash_subject.bats diff --git a/.github/scripts/check-squash-subject.sh b/.github/scripts/check-squash-subject.sh new file mode 100755 index 000000000..506ab703b --- /dev/null +++ b/.github/scripts/check-squash-subject.sh @@ -0,0 +1,141 @@ +#!/usr/bin/env bash +# +# Fail when the subject a squash merge WILL write is one git-cliff would drop. +# +# The repository squashes every PR (allow_merge_commit=false, +# allow_rebase_merge=false) with squash_merge_commit_title=COMMIT_OR_PR_TITLE. +# Measured on the 30 most recent PRs merged into main (2026-09-07): a PR with +# ONE commit lands under that commit's subject (3 of 3 cases where the two +# differed), a PR with two or more lands under the PR title (5 of 5). So the +# thing to check is not "the PR title" but "whichever of the two will become +# the subject", and this script asks for both inputs so it can pick. +# +# Why the subject matters: cliff.toml's commit_parsers classify a subject by an +# anchored prefix (`^feat`, `^fix`, ...) and its last rule skips everything +# else. A subject without one of those prefixes is silently absent from the +# release notes -- 1.1.7 lost a headline feature that way, and on the 1.3.0 +# integration branch four more landed prefix-less: +# +# Naming is an invariant every entry point re-asserts ... (#1047) +# Remove internal handles from the published tree ... (#1062) +# spawn: launch codex through the bundled monitor shim ... (#1064) +# Reduce repeated ciphertext literals in pull apply (#1043, main) +# +# `spawn:` is the instructive one: it has the SHAPE of a type but is not one +# cliff.toml classifies, so shape alone is not the test. The accepted types +# are therefore DERIVED from cliff.toml at run time, never retyped here; a +# type added to cliff.toml is accepted by the next run of this script. +# +# What this checks, in order: +# 1. cliff.toml still has parsers of the shape this reads (canary: the derived +# set must name feat and fix). If not, exit 2 -- a checker that cannot +# derive its rule has nothing to be green about. +# 2. There is a subject to check. No subject, no commit count, an unreadable +# head: exit 2, not 0. Scanning nothing is not a pass. +# 3. The subject matches ()?!?: with in the +# derived set. Otherwise exit 1, saying which subject was checked, why +# that one, and what cliff would do with it. +# +# Usage (CI passes the first form; the second is for a pre-PR check by hand): +# check-squash-subject.sh --title "" --commits --head [--repo ] +# check-squash-subject.sh --subject "" +# +# --repo is where is read from (default: this checkout), so the +# selection can be exercised against a fixture repository. +# +# Environment: +# AGMSG_CLIFF_CONFIG path to cliff.toml (default: /cliff.toml), so the +# derivation can be exercised against a fixture. + +set -u + +ROOT="$(cd "$(dirname "$0")/../.." && pwd)" +CLIFF="${AGMSG_CLIFF_CONFIG:-$ROOT/cliff.toml}" +ME=check-squash-subject + +title=""; commits=""; head=""; subject=""; subject_source=""; repo="$ROOT" +while [ $# -gt 0 ]; do + case "$1" in + --title) title="${2-}"; shift 2 ;; + --commits) commits="${2-}"; shift 2 ;; + --head) head="${2-}"; shift 2 ;; + --repo) repo="${2-}"; shift 2 ;; + --subject) subject="${2-}"; subject_source="the subject given on the command line"; shift 2 ;; + -h|--help) sed -n '2,45p' "$0" | sed 's/^# \{0,1\}//'; exit 0 ;; + *) echo "$ME: unknown argument: $1" >&2; exit 2 ;; + esac +done + +# --- 1. derive the accepted types from cliff.toml ---------------------------- +# +# A parser line looks like { message = "^feat", group = "..." } or +# { message = "^(chore|ci|build|test|style)", skip = true }. Take every +# anchored pattern, keep the lowercase words at its start (a bare word or an +# alternation), and that is the set. Anything else in the file -- exact legacy +# titles, `^Merge`, the `.*` catch-all -- is not a type and is not collected. +if [ ! -r "$CLIFF" ]; then + echo "$ME: cannot read $CLIFF; the accepted types are derived from it, so nothing can be checked." >&2 + exit 2 +fi +types="$( + grep -oE 'message *= *"\^(\([a-z|]+\)|[a-z]+)"' "$CLIFF" \ + | sed -E 's/.*"\^//; s/[()"]//g' | tr '|' '\n' | sed '/^$/d' | sort -u +)" +for canary in feat fix; do + if ! printf '%s\n' "$types" | grep -qx "$canary"; then + echo "$ME: the types derived from $CLIFF do not include '$canary' -- the file no longer has the shape this reads. Derived: $(printf '%s' "$types" | tr '\n' ' ')" >&2 + exit 2 + fi +done +alternation="$(printf '%s\n' "$types" | paste -sd '|' -)" + +# --- 2. pick the subject a squash merge would write --------------------------- +if [ -z "$subject" ]; then + if [ -z "$title" ] && [ -z "$commits" ] && [ -z "$head" ]; then + echo "$ME: no --subject and no --title/--commits/--head; nothing to check is not a pass." >&2 + exit 2 + fi + case "$commits" in + ''|*[!0-9]*|0) + echo "$ME: --commits must be the PR's commit count (got '${commits}'); without it the landing subject cannot be chosen." >&2 + exit 2 ;; + esac + if [ "$commits" -eq 1 ]; then + if [ -z "$head" ]; then + echo "$ME: a one-commit PR lands under its commit's subject, and no --head was given to read it from." >&2 + exit 2 + fi + if ! subject="$(git -C "$repo" log -1 --format=%s "$head" 2>/dev/null)" || [ -z "$subject" ]; then + echo "$ME: cannot read the subject of $head in $repo (not fetched?); a one-commit PR lands under that subject." >&2 + exit 2 + fi + subject_source="the single commit's subject ($head), which COMMIT_OR_PR_TITLE uses for a one-commit PR" + else + if [ -z "$title" ]; then + echo "$ME: a $commits-commit PR lands under the PR title, and no --title was given." >&2 + exit 2 + fi + subject="$title" + subject_source="the PR title, which COMMIT_OR_PR_TITLE uses for a $commits-commit PR" + fi +fi + +# --- 3. the rule ---------------------------------------------------------------- +if printf '%s\n' "$subject" | grep -Eq "^($alternation)(\([^)]+\))?!?: [^ ]"; then + echo "$ME: ok -- checked $subject_source:" + echo " $subject" + exit 0 +fi + +echo "$ME: this subject would be dropped from the release notes." >&2 +echo >&2 +echo " $subject" >&2 +echo >&2 +echo "Checked: $subject_source." >&2 +echo "cliff.toml classifies a subject by its prefix and skips everything else, so" >&2 +echo "the landed subject must look like ()?!?: with " >&2 +echo "one of (derived from cliff.toml):" >&2 +echo " $(printf '%s' "$types" | tr '\n' ' ')" >&2 +echo "Retitle the PR, or for a one-commit PR reword that commit (or add a second" >&2 +echo "commit so the PR title is what lands)." >&2 +exit 1 diff --git a/.github/workflows/squash-subject.yml b/.github/workflows/squash-subject.yml new file mode 100644 index 000000000..6f8ac168c --- /dev/null +++ b/.github/workflows/squash-subject.yml @@ -0,0 +1,51 @@ +# The subject a squash merge will write must be one git-cliff keeps. +# +# This repository squashes every PR with squash_merge_commit_title set to +# COMMIT_OR_PR_TITLE: a one-commit PR lands under its commit's subject, a +# larger one under the PR title (measured, see the script's header). A subject +# without a prefix cliff.toml classifies is silently absent from the release +# notes; that has happened to a headline feature (1.1.7) and to four more +# landings on the 1.3.0 integration branch. This runs the check on the subject +# that would actually land. +# +# `edited` is in the event types on purpose: a PR title is fixed by editing it, +# and without `edited` the fix would never be re-checked (nor would a title +# broken after the last push). The other types are the defaults plus +# ready_for_review, spelled out because naming `edited` replaces the default +# list rather than adding to it. +# +# This is its own workflow rather than a job in tests.yml because `edited` +# would otherwise re-run the whole test matrix on every title edit. + +name: squash-subject + +on: + pull_request: + types: [opened, edited, synchronize, reopened, ready_for_review] + branches: [main, 'integration/**'] + +permissions: + contents: read + +jobs: + check: + name: squash subject has a type cliff.toml keeps + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + # The PR head itself, not the merge ref: for a one-commit PR the script + # reads that commit's subject from git, and the merge ref's parents are + # not present at depth 1. + - uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.head.sha }} + fetch-depth: 1 + + - name: The subject that would land has a type cliff.toml keeps + env: + PR_TITLE: ${{ github.event.pull_request.title }} + PR_COMMITS: ${{ github.event.pull_request.commits }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + .github/scripts/check-squash-subject.sh \ + --title "$PR_TITLE" --commits "$PR_COMMITS" --head "$HEAD_SHA" diff --git a/tests/test_check_squash_subject.bats b/tests/test_check_squash_subject.bats new file mode 100644 index 000000000..375459a95 --- /dev/null +++ b/tests/test_check_squash_subject.bats @@ -0,0 +1,216 @@ +#!/usr/bin/env bats +# +# .github/scripts/check-squash-subject.sh: the subject a squash merge will +# write must carry a type cliff.toml classifies, or git-cliff drops it from the +# release notes. Three things are pinned here, each with a control in the +# other direction: +# +# - the RULE: which subjects pass, including the ones that already landed +# wrong (they must be red -- a checker that accepts the very cases it was +# written for has never fired); +# - the SELECTION: a one-commit PR is judged by its commit's subject, a larger +# PR by its title (measured on merged PRs, see the script header); +# - the ZERO-TARGET answer: no subject, no commit count, an unreadable head, +# or a cliff.toml the derivation cannot read must be exit 2, never 0. + +setup() { + load 'test_helper' + REPO_ROOT="$(cd "$BATS_TEST_DIRNAME/.." && pwd)" + CHECK="$REPO_ROOT/.github/scripts/check-squash-subject.sh" + export AGMSG_CLIFF_CONFIG="$REPO_ROOT/cliff.toml" +} + +# A one-line cliff.toml fixture with exactly the parsers given, so the +# derivation can be pointed at a file whose contents the test controls. +_cliff_fixture() { # + local f="$BATS_TEST_TMPDIR/cliff.toml" p + { + echo '[git]' + echo 'commit_parsers = [' + for p in "$@"; do printf ' { message = "%s", group = "x" },\n' "$p"; done + echo ' { message = ".*", skip = true },' + echo ']' + } > "$f" + printf '%s' "$f" +} + +# A throwaway repository with one commit whose subject is given; prints the sha. +_repo_with_commit() { # + local d="$BATS_TEST_TMPDIR/repo" + git init -q "$d" + git -C "$d" -c user.name=t -c user.email=t@x commit -q --allow-empty -m "$1" + git -C "$d" rev-parse HEAD +} + +# --- the rule --------------------------------------------------------------------- + +@test "each subject that already landed prefix-less is red" { + # The cases this exists for. Measured on integration/terminal-driver-v1 and + # main (2026-09-07); `spawn:` has the shape of a type and is not one. + local s + while IFS= read -r s; do + run bash "$CHECK" --subject "$s" + [ "$status" -eq 1 ] || { echo "accepted: $s"; return 1; } + grep -q 'dropped from the release notes' <<<"$output" || { echo "no reason for: $s"; return 1; } + done <<'EOF' +Naming is an invariant every entry point re-asserts, and the roster can see it (#1044) +Remove internal handles from the published tree, and check for them by identifier context +spawn: launch codex through the bundled monitor shim, and refuse a bare fallback +Reduce repeated ciphertext literals in pull apply +EOF +} + +@test "a subject with a type cliff.toml keeps is green, scope and bang included" { + local s + while IFS= read -r s; do + run bash "$CHECK" --subject "$s" + [ "$status" -eq 0 ] || { echo "rejected: $s -- $output"; return 1; } + done <<'EOF' +feat(spawn): set the pane's agent key at spawn so codex seats are not left nameless +fix: refuse a non-string envelope.blob +perf(sync): check a pulled wire id without a process +release: 1.3.0 +chore(ci): pin the runner image +feat!: drop the legacy index +EOF +} + +@test "the shape is exact: unknown type, missing space, wrong case are red" { + local s + while IFS= read -r s; do + run bash "$CHECK" --subject "$s" + [ "$status" -eq 1 ] || { echo "accepted: $s"; return 1; } + done <<'EOF' +feature-flag: gate the new path +feat:no space after the colon +Feat: capitalised type +feat (spawn): space before the scope +EOF +} + +@test "the type set is derived from cliff.toml, not retyped: a new type is accepted without editing the script" { + # Differential pair on the same subject; only the fixture differs. + export AGMSG_CLIFF_CONFIG + AGMSG_CLIFF_CONFIG="$(_cliff_fixture '^feat' '^fix')" + run bash "$CHECK" --subject 'wibble: a subject of a type nobody has yet' + [ "$status" -eq 1 ] + AGMSG_CLIFF_CONFIG="$(_cliff_fixture '^feat' '^fix' '^(wibble|wobble)')" + run bash "$CHECK" --subject 'wibble: a subject of a type nobody has yet' + [ "$status" -eq 0 ] +} + +@test "the derivation reads the real cliff.toml the same way an independent scrape does" { + # Canary for the sed in the script: a second, cruder scrape of the same file + # must agree, and both must find a type that lives only in an alternation + # (`chore`) and one that is skipped rather than grouped (`release`). + local expected got + expected="$(grep -oE 'message *= *"\^[a-z(][a-z|)]*' "$REPO_ROOT/cliff.toml" \ + | sed -E 's/.*"\^//; s/[()]//g' | tr '|' '\n' | sort -u)" + got="$(bash "$CHECK" --subject 'not: a real one' 2>&1 >/dev/null | grep -A1 'derived from cliff.toml' | tail -1 | tr ' ' '\n' | sed '/^$/d' | sort -u)" + [ "$got" = "$expected" ] + grep -qx chore <<<"$got" + grep -qx release <<<"$got" +} + +# --- the selection: which subject would land ------------------------------------ + +@test "a one-commit PR is judged by its commit subject, even when the PR title is fine" { + # The asymmetric case that a title-only check gets wrong: #1043 landed + # prefix-less under a fine-looking review, because the commit is what lands. + local sha + sha="$(_repo_with_commit 'Reduce repeated ciphertext literals in pull apply')" + run bash "$CHECK" --title 'fix(sync): reduce repeated ciphertext literals' --commits 1 --head "$sha" --repo "$BATS_TEST_TMPDIR/repo" + [ "$status" -eq 1 ] + grep -q "single commit's subject" <<<"$output" + grep -q 'Reduce repeated ciphertext literals' <<<"$output" +} + +@test "a one-commit PR with a good commit subject is green even when the title is bad" { + local sha + sha="$(_repo_with_commit 'fix(sync): reduce repeated ciphertext literals')" + run bash "$CHECK" --title 'Reduce repeated ciphertext literals in pull apply' --commits 1 --head "$sha" --repo "$BATS_TEST_TMPDIR/repo" + [ "$status" -eq 0 ] + grep -q "single commit's subject" <<<"$output" +} + +@test "a multi-commit PR is judged by its title, and the commit subjects do not matter" { + local sha + sha="$(_repo_with_commit 'wip')" + run bash "$CHECK" --title 'feat(spawn): set the agent key at spawn' --commits 2 --head "$sha" --repo "$BATS_TEST_TMPDIR/repo" + [ "$status" -eq 0 ] + grep -q 'the PR title' <<<"$output" + run bash "$CHECK" --title 'Set the agent key at spawn' --commits 2 --head "$sha" --repo "$BATS_TEST_TMPDIR/repo" + [ "$status" -eq 1 ] + grep -q 'the PR title' <<<"$output" +} + +@test "the verdict names what it checked, in both directions" { + # A green that does not say which subject it read is indistinguishable from + # a green that read nothing. + run bash "$CHECK" --subject 'fix: something' + [ "$status" -eq 0 ] + grep -q 'checked the subject given on the command line' <<<"$output" + run bash "$CHECK" --subject 'something' + [ "$status" -eq 1 ] + grep -q '^Checked: the subject given on the command line' <<<"$output" +} + +# --- zero targets are not a pass -------------------------------------------------- + +@test "no subject at all is exit 2, not green" { + run bash "$CHECK" + [ "$status" -eq 2 ] + grep -q 'nothing to check is not a pass' <<<"$output" +} + +@test "a missing or non-numeric commit count is exit 2" { + run bash "$CHECK" --title 'fix: fine' --head deadbeef + [ "$status" -eq 2 ] + run bash "$CHECK" --title 'fix: fine' --commits many --head deadbeef + [ "$status" -eq 2 ] + run bash "$CHECK" --title 'fix: fine' --commits 0 --head deadbeef + [ "$status" -eq 2 ] +} + +@test "a one-commit PR whose head cannot be read is exit 2, not judged by the title" { + # The title is fine here on purpose: falling back to it would be a green + # about a subject that was never read. + run bash "$CHECK" --title 'fix: fine' --commits 1 --head 0000000000000000000000000000000000000000 + [ "$status" -eq 2 ] + grep -q 'cannot read the subject' <<<"$output" + run bash "$CHECK" --title 'fix: fine' --commits 1 + [ "$status" -eq 2 ] +} + +@test "a multi-commit PR with no title is exit 2" { + run bash "$CHECK" --commits 3 --head deadbeef + [ "$status" -eq 2 ] +} + +@test "a cliff.toml the derivation cannot read is exit 2 even for a good subject" { + # Differential pair on one good subject: only the config differs. The first + # has no parsers at all; the second has parsers but not the canaries. + export AGMSG_CLIFF_CONFIG + AGMSG_CLIFF_CONFIG="$BATS_TEST_TMPDIR/missing.toml" + run bash "$CHECK" --subject 'feat: fine' + [ "$status" -eq 2 ] + AGMSG_CLIFF_CONFIG="$(_cliff_fixture '^(chore|ci)')" + run bash "$CHECK" --subject 'feat: fine' + [ "$status" -eq 2 ] + grep -q "do not include 'feat'" <<<"$output" + AGMSG_CLIFF_CONFIG="$(_cliff_fixture '^feat' '^fix')" + run bash "$CHECK" --subject 'feat: fine' + [ "$status" -eq 0 ] +} + +@test "the workflow passes the three inputs the selection needs, and re-runs on a title edit" { + # The script can only pick the landing subject if CI hands it the commit + # count and the head; and a title fixed by editing is only re-checked if + # `edited` is among the event types. + local wf="$REPO_ROOT/.github/workflows/squash-subject.yml" + grep -q 'check-squash-subject.sh' "$wf" + grep -q 'github.event.pull_request.commits' "$wf" + grep -q 'github.event.pull_request.head.sha' "$wf" + grep -q 'github.event.pull_request.title' "$wf" + grep -Eq 'types: \[.*edited.*\]' "$wf" +} From 0fabad46f9cd1a7f87b3e532a1d4f40dd6c64f3e Mon Sep 17 00:00:00 2001 From: fujibee Date: Mon, 7 Sep 2026 02:23:04 -0700 Subject: [PATCH 2/4] fix(ci): judge the squash subject by what git-cliff does, not by its 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. --- .github/scripts/check-squash-subject.sh | 235 +++++++++++++++------ tests/test_check_squash_subject.bats | 263 +++++++++++++++++------- 2 files changed, 362 insertions(+), 136 deletions(-) diff --git a/.github/scripts/check-squash-subject.sh b/.github/scripts/check-squash-subject.sh index 506ab703b..c4596f727 100755 --- a/.github/scripts/check-squash-subject.sh +++ b/.github/scripts/check-squash-subject.sh @@ -2,6 +2,12 @@ # # Fail when the subject a squash merge WILL write is one git-cliff would drop. # +# This checker decides KEEP or DROP -- what git-cliff does with the subject +# under cliff.toml. It does not bind new subjects to the Conventional shape, +# and the two must not be mixed: a Conventional subject can be dropped +# (`chore:` hits a skip rule) and a non-Conventional one can be kept (the +# legacy exceptions in cliff.toml), and a check on the shape gets both wrong. +# # The repository squashes every PR (allow_merge_commit=false, # allow_rebase_merge=false) with squash_merge_commit_title=COMMIT_OR_PR_TITLE. # Measured on the 30 most recent PRs merged into main (2026-09-07): a PR with @@ -10,35 +16,45 @@ # thing to check is not "the PR title" but "whichever of the two will become # the subject", and this script asks for both inputs so it can pick. # -# Why the subject matters: cliff.toml's commit_parsers classify a subject by an -# anchored prefix (`^feat`, `^fix`, ...) and its last rule skips everything -# else. A subject without one of those prefixes is silently absent from the -# release notes -- 1.1.7 lost a headline feature that way, and on the 1.3.0 -# integration branch four more landed prefix-less: +# Why the subject matters: a subject git-cliff drops is silently absent from +# the release notes -- 1.1.7 lost a headline feature that way, and on the 1.3.0 +# integration branch four more landed that way (#1047, #1062, #1064 and, on +# main, #1043). # -# Naming is an invariant every entry point re-asserts ... (#1047) -# Remove internal handles from the published tree ... (#1062) -# spawn: launch codex through the bundled monitor shim ... (#1064) -# Reduce repeated ciphertext literals in pull apply (#1043, main) +# What git-cliff does, MEASURED with git-cliff 2.10.1 against this cliff.toml +# on a fixture repository (2026-09-07): +# - commit_parsers are tried in file order and the FIRST match decides: +# `chore: native windows bits` is dropped by the `^(chore|...)` skip rule +# even though `(?i)native windows` would keep it; +# - a rule with `group` keeps the commit, a rule with `skip = true` drops it, +# and the trailing `.*` skip rule drops everything unmatched; +# - matching is a regex SEARCH over the message, so `^feat` also keeps +# `feature-flag: ...`, and `Feat:` (capitalised) is dropped; +# - unanchored patterns see the commit BODY too: a subject with no match +# whose body says "native windows" is kept. This script judges the SUBJECT +# LINE only, because that line is what the notes show and what a reviewer +# reads; the divergence is confined to cliff.toml's unanchored legacy +# rules, and it errs towards red; +# - with `filter_unconventional` absent, git-cliff's default drops every +# non-Conventional commit before the parsers run; this cliff.toml sets it +# to false explicitly, and the script honours whichever it finds. # -# `spawn:` is the instructive one: it has the SHAPE of a type but is not one -# cliff.toml classifies, so shape alone is not the test. The accepted types -# are therefore DERIVED from cliff.toml at run time, never retyped here; a -# type added to cliff.toml is accepted by the next run of this script. +# The rules are DERIVED from cliff.toml at run time, never retyped here; a +# rule added there is honoured by the next run. # # What this checks, in order: -# 1. cliff.toml still has parsers of the shape this reads (canary: the derived -# set must name feat and fix). If not, exit 2 -- a checker that cannot -# derive its rule has nothing to be green about. +# 1. cliff.toml can be read and has parsers of the shape this reads (at +# least one `group` rule and at least one `skip` rule). If not, exit 2 -- +# a checker that cannot derive its rule has nothing to be green about. # 2. There is a subject to check. No subject, no commit count, an unreadable # head: exit 2, not 0. Scanning nothing is not a pass. -# 3. The subject matches ()?!?: with in the -# derived set. Otherwise exit 1, saying which subject was checked, why -# that one, and what cliff would do with it. +# 3. The first cliff.toml rule matching the subject has a `group`: exit 0. +# A skip rule, or no rule at all: exit 1, naming the rule and the subject. # # Usage (CI passes the first form; the second is for a pre-PR check by hand): # check-squash-subject.sh --title "" --commits --head [--repo ] # check-squash-subject.sh --subject "" +# check-squash-subject.sh --parsers # print the rules as derived, in order # # --repo is where is read from (default: this checkout), so the # selection can be exercised against a fixture repository. @@ -53,41 +69,100 @@ ROOT="$(cd "$(dirname "$0")/../.." && pwd)" CLIFF="${AGMSG_CLIFF_CONFIG:-$ROOT/cliff.toml}" ME=check-squash-subject -title=""; commits=""; head=""; subject=""; subject_source=""; repo="$ROOT" +title=""; commits=""; head=""; subject=""; subject_source=""; repo="$ROOT"; mode=judge while [ $# -gt 0 ]; do case "$1" in - --title) title="${2-}"; shift 2 ;; - --commits) commits="${2-}"; shift 2 ;; - --head) head="${2-}"; shift 2 ;; - --repo) repo="${2-}"; shift 2 ;; - --subject) subject="${2-}"; subject_source="the subject given on the command line"; shift 2 ;; - -h|--help) sed -n '2,45p' "$0" | sed 's/^# \{0,1\}//'; exit 0 ;; + --title|--commits|--head|--repo|--subject) + # An option without its value must end here, not loop: `shift 2` with + # one argument left fails without shifting, and this loop has no errexit. + if [ $# -lt 2 ]; then + echo "$ME: $1 needs a value." >&2 + exit 2 + fi ;; + esac + case "$1" in + --title) title="$2"; shift 2 ;; + --commits) commits="$2"; shift 2 ;; + --head) head="$2"; shift 2 ;; + --repo) repo="$2"; shift 2 ;; + --subject) subject="$2"; subject_source="the subject given on the command line"; shift 2 ;; + --parsers) mode=parsers; shift ;; + -h|--help) sed -n '2,62p' "$0" | sed 's/^# \{0,1\}//'; exit 0 ;; *) echo "$ME: unknown argument: $1" >&2; exit 2 ;; esac done -# --- 1. derive the accepted types from cliff.toml ---------------------------- +# --- 1. derive the rules from cliff.toml ----------------------------------------- +# +# Reads the `commit_parsers = [ ... ]` array of the [git] table line by line: +# one `{ message = "", group = "..." }` or `{ ..., skip = true }` per +# line, as the file is written. No TOML library, so it runs on any python3. +# Also reads `filter_unconventional` (git-cliff's default when absent: true, +# measured) and `conventional_commits`. # -# A parser line looks like { message = "^feat", group = "..." } or -# { message = "^(chore|ci|build|test|style)", skip = true }. Take every -# anchored pattern, keep the lowercase words at its start (a bare word or an -# alternation), and that is the set. Anything else in the file -- exact legacy -# titles, `^Merge`, the `.*` catch-all -- is not a type and is not collected. -if [ ! -r "$CLIFF" ]; then - echo "$ME: cannot read $CLIFF; the accepted types are derived from it, so nothing can be checked." >&2 +# Prints, one per line: \t\t and, first, a +# header line filter_unconventional=. Exits 2 when the file has +# no usable rules. +derive_rules() { + python3 - "$1" <<'PY' +import re, sys, pathlib +p = pathlib.Path(sys.argv[1]) +try: + text = p.read_text() +except OSError as e: + print(f"cannot read {p}: {e}", file=sys.stderr); sys.exit(2) +# Only the [git] table matters; stop at the next table header. +m = re.search(r'^\[git\]\s*$(.*?)(?=^\[|\Z)', text, re.S | re.M) +git = m.group(1) if m else '' +def flag(name, default): + mm = re.search(r'^\s*' + name + r'\s*=\s*(true|false)\b', git, re.M) + return (mm.group(1) == 'true') if mm else default +filter_unconventional = flag('filter_unconventional', True) +# The parsers array: everything between `commit_parsers = [` and the closing `]` +# that starts a line. +a = re.search(r'^\s*commit_parsers\s*=\s*\[(.*?)^\s*\]', git, re.S | re.M) +rules = [] +if a: + for line in a.group(1).splitlines(): + s = line.strip() + if not s or s.startswith('#'): + continue + mm = re.search(r'message\s*=\s*"((?:[^"\\]|\\.)*)"', s) + if not mm: + continue + pattern = bytes(mm.group(1), 'utf-8').decode('unicode_escape') + if re.search(r'\bskip\s*=\s*true\b', s): + kind = 'skip' + elif re.search(r'\bgroup\s*=', s): + kind = 'keep' + else: + continue + rules.append((kind, pattern)) +keeps = sum(1 for k, _ in rules if k == 'keep') +skips = sum(1 for k, _ in rules if k == 'skip') +if not rules or keeps == 0 or skips == 0: + print(f"{p} has no usable commit_parsers (keep rules: {keeps}, skip rules: {skips}); " + f"the file no longer has the shape this reads.", file=sys.stderr) + sys.exit(2) +for k, pat in rules: + try: + re.compile(pat) + except re.error as e: + print(f"{p}: cannot compile parser pattern {pat!r}: {e}", file=sys.stderr); sys.exit(2) +print(f"filter_unconventional={'true' if filter_unconventional else 'false'}") +for i, (k, pat) in enumerate(rules, 1): + print(f"{i}\t{k}\t{pat}") +PY +} + +if ! rules="$(derive_rules "$CLIFF")"; then + echo "$ME: cannot derive the rules from $CLIFF; nothing can be checked." >&2 exit 2 fi -types="$( - grep -oE 'message *= *"\^(\([a-z|]+\)|[a-z]+)"' "$CLIFF" \ - | sed -E 's/.*"\^//; s/[()"]//g' | tr '|' '\n' | sed '/^$/d' | sort -u -)" -for canary in feat fix; do - if ! printf '%s\n' "$types" | grep -qx "$canary"; then - echo "$ME: the types derived from $CLIFF do not include '$canary' -- the file no longer has the shape this reads. Derived: $(printf '%s' "$types" | tr '\n' ' ')" >&2 - exit 2 - fi -done -alternation="$(printf '%s\n' "$types" | paste -sd '|' -)" +if [ "$mode" = parsers ]; then + printf '%s\n' "$rules" + exit 0 +fi # --- 2. pick the subject a squash merge would write --------------------------- if [ -z "$subject" ]; then @@ -120,22 +195,56 @@ if [ -z "$subject" ]; then fi fi -# --- 3. the rule ---------------------------------------------------------------- -if printf '%s\n' "$subject" | grep -Eq "^($alternation)(\([^)]+\))?!?: [^ ]"; then - echo "$ME: ok -- checked $subject_source:" - echo " $subject" - exit 0 -fi +# --- 3. what git-cliff would do with it ------------------------------------------ +# +# Prints one line: keep|drop -- the first matching rule in +# file order decides, as measured. The rules travel in the environment: the +# python program itself is what `python3 -` reads from stdin. (A function, not +# an inline `$( ... <<'PY' )`: bash 3.2 mis-parses a heredoc with an unbalanced +# parenthesis inside command substitution.) +judge_subject() { # $1 = subject; RULES in the environment + python3 - "$1" <<'PY' +import os, re, sys +subject = sys.argv[1] +lines = os.environ['RULES'].splitlines() +filter_unconventional = lines[0].split('=', 1)[1] == 'true' +rules = [l.split('\t', 2) for l in lines[1:]] +# git-cliff's `filter_unconventional` drops a non-Conventional commit before +# any parser runs. The shape it parses is ()?!?: . +if filter_unconventional and not re.match(r'^[A-Za-z][A-Za-z0-9_-]*(\([^()]*\))?!?: \S', subject): + print("drop\tcliff.toml has filter_unconventional=true (git-cliff's default when absent), " + "and this subject is not of the form ()?!?: , so it is dropped before any parser runs") + sys.exit(0) +for i, kind, pat in rules: + if re.search(pat, subject): + if kind == 'keep': + print(f"keep\tkept by parser {i} `{pat}`") + else: + print(f"drop\tdropped by parser {i} `{pat}` (skip = true)") + sys.exit(0) +print("drop\tmatches none of cliff.toml's parsers") +PY +} +verdict="$(RULES="$rules" judge_subject "$subject")" +decision="${verdict%% *}" +reason="${verdict#* }" -echo "$ME: this subject would be dropped from the release notes." >&2 -echo >&2 -echo " $subject" >&2 -echo >&2 -echo "Checked: $subject_source." >&2 -echo "cliff.toml classifies a subject by its prefix and skips everything else, so" >&2 -echo "the landed subject must look like ()?!?: with " >&2 -echo "one of (derived from cliff.toml):" >&2 -echo " $(printf '%s' "$types" | tr '\n' ' ')" >&2 -echo "Retitle the PR, or for a one-commit PR reword that commit (or add a second" >&2 -echo "commit so the PR title is what lands)." >&2 -exit 1 +case "$decision" in + keep) + echo "$ME: ok -- git-cliff keeps it ($reason). Checked $subject_source:" + echo " $subject" + exit 0 ;; + drop) + echo "$ME: this subject would be dropped from the release notes: $reason." >&2 + echo >&2 + echo " $subject" >&2 + echo >&2 + echo "Checked: $subject_source." >&2 + echo "The rules are cliff.toml's commit_parsers, first match wins; \`$0 --parsers\`" >&2 + echo "prints them as derived. Retitle the PR, or for a one-commit PR reword that" >&2 + echo "commit (or add a second commit so the PR title is what lands)." >&2 + exit 1 ;; + *) + echo "$ME: internal error: no verdict for the subject." >&2 + exit 2 ;; +esac diff --git a/tests/test_check_squash_subject.bats b/tests/test_check_squash_subject.bats index 375459a95..f906c8c8d 100644 --- a/tests/test_check_squash_subject.bats +++ b/tests/test_check_squash_subject.bats @@ -1,17 +1,22 @@ #!/usr/bin/env bats # # .github/scripts/check-squash-subject.sh: the subject a squash merge will -# write must carry a type cliff.toml classifies, or git-cliff drops it from the -# release notes. Three things are pinned here, each with a control in the -# other direction: +# write must be one git-cliff KEEPS under cliff.toml, or it is silently absent +# from the release notes. The checker decides keep/drop; it does not bind new +# subjects to the Conventional shape, and this file pins both directions of +# that distinction. Four things are pinned, each with a control the other way: # -# - the RULE: which subjects pass, including the ones that already landed -# wrong (they must be red -- a checker that accepts the very cases it was -# written for has never fired); +# - the RULE, against what git-cliff itself does (measured, and re-measured +# here whenever git-cliff is installed): subjects that already landed and +# were dropped are red, kept ones are green, a skip rule reddens a +# Conventional subject and a legacy group rule greens a non-Conventional one; +# - the DERIVATION: the rules the script reads out of cliff.toml agree with +# an independent scrape, in order and in kind; # - the SELECTION: a one-commit PR is judged by its commit's subject, a larger # PR by its title (measured on merged PRs, see the script header); # - the ZERO-TARGET answer: no subject, no commit count, an unreadable head, -# or a cliff.toml the derivation cannot read must be exit 2, never 0. +# an option without a value, or a cliff.toml the derivation cannot read +# must be exit 2, never 0. setup() { load 'test_helper' @@ -20,15 +25,48 @@ setup() { export AGMSG_CLIFF_CONFIG="$REPO_ROOT/cliff.toml" } -# A one-line cliff.toml fixture with exactly the parsers given, so the -# derivation can be pointed at a file whose contents the test controls. -_cliff_fixture() { # - local f="$BATS_TEST_TMPDIR/cliff.toml" p +# The subjects this file judges, with git-cliff's own verdict for each under +# the real cliff.toml (measured with git-cliff 2.10.1, 2026-09-07). One table, +# read by the rule tests AND by the git-cliff cross-check, so the two cannot +# drift apart. +_subject_table() { + cat <<'EOF' +keep|feat(spawn): set the pane's agent key at spawn so codex seats are not left nameless +keep|fix: refuse a non-string envelope.blob +keep|perf(sync): check a pulled wire id without a process +keep|feat!: drop the legacy index +keep|Add native Windows support for the launcher +keep|Role-to-session affinity: pin the seat +keep|feature-flag: kept by the ^feat rule, Conventional or not +drop|Naming is an invariant every entry point re-asserts, and the roster can see it (#1044) +drop|Remove internal handles from the published tree, and check for them by identifier context +drop|spawn: launch codex through the bundled monitor shim, and refuse a bare fallback +drop|Reduce repeated ciphertext literals in pull apply +drop|chore(ci): pin the runner image +drop|release: 1.3.0 +drop|ci: a Conventional subject that a skip rule drops +drop|chore: native windows bits +drop|Feat: capitalised +drop|Something unrelated in the subject +EOF +} + +# A one-line cliff.toml fixture with exactly the parsers given (`skip:` or +# `keep:`), preceded by any extra [git] lines, so the derivation can be +# pointed at a file whose contents the test controls. +_cliff_fixture() { # ... + local f="$BATS_TEST_TMPDIR/cliff-$RANDOM.toml" extra="$1" r + shift { echo '[git]' + [ -z "$extra" ] || printf '%s\n' "$extra" echo 'commit_parsers = [' - for p in "$@"; do printf ' { message = "%s", group = "x" },\n' "$p"; done - echo ' { message = ".*", skip = true },' + for r in "$@"; do + case "$r" in + skip:*) printf ' { message = "%s", skip = true },\n' "${r#skip:}" ;; + keep:*) printf ' { message = "%s", group = "x" },\n' "${r#keep:}" ;; + esac + done echo ']' } > "$f" printf '%s' "$f" @@ -42,81 +80,134 @@ _repo_with_commit() { # git -C "$d" rev-parse HEAD } -# --- the rule --------------------------------------------------------------------- +# --- the rule, both directions ------------------------------------------------------ -@test "each subject that already landed prefix-less is red" { - # The cases this exists for. Measured on integration/terminal-driver-v1 and - # main (2026-09-07); `spawn:` has the shape of a type and is not one. - local s - while IFS= read -r s; do +@test "every subject in the table gets git-cliff's measured verdict: kept is green, dropped is red" { + local exp s rc + while IFS='|' read -r exp s; do run bash "$CHECK" --subject "$s" - [ "$status" -eq 1 ] || { echo "accepted: $s"; return 1; } - grep -q 'dropped from the release notes' <<<"$output" || { echo "no reason for: $s"; return 1; } - done <<'EOF' -Naming is an invariant every entry point re-asserts, and the roster can see it (#1044) -Remove internal handles from the published tree, and check for them by identifier context -spawn: launch codex through the bundled monitor shim, and refuse a bare fallback -Reduce repeated ciphertext literals in pull apply -EOF + case "$exp" in keep) rc=0 ;; drop) rc=1 ;; esac + [ "$status" -eq "$rc" ] || { echo "expected $exp (exit $rc), got exit $status for: $s"; echo "$output"; return 1; } + done < <(_subject_table) } -@test "a subject with a type cliff.toml keeps is green, scope and bang included" { - local s - while IFS= read -r s; do - run bash "$CHECK" --subject "$s" - [ "$status" -eq 0 ] || { echo "rejected: $s -- $output"; return 1; } - done <<'EOF' -feat(spawn): set the pane's agent key at spawn so codex seats are not left nameless -fix: refuse a non-string envelope.blob -perf(sync): check a pulled wire id without a process -release: 1.3.0 -chore(ci): pin the runner image -feat!: drop the legacy index -EOF +@test "the table itself has both directions and both distinctions" { + # A table with only one direction would make the test above vacuous. It must + # hold a Conventional subject that is DROPPED (skip rule) and a + # non-Conventional one that is KEPT (legacy group rule). + _subject_table | grep -q '^drop|chore(ci):' + _subject_table | grep -q '^drop|release:' + _subject_table | grep -q '^keep|Add native Windows' + _subject_table | grep -q '^keep|Role-to-session affinity' + [ "$(_subject_table | grep -c '^keep|')" -ge 5 ] + [ "$(_subject_table | grep -c '^drop|')" -ge 5 ] } -@test "the shape is exact: unknown type, missing space, wrong case are red" { - local s - while IFS= read -r s; do - run bash "$CHECK" --subject "$s" - [ "$status" -eq 1 ] || { echo "accepted: $s"; return 1; } - done <<'EOF' -feature-flag: gate the new path -feat:no space after the colon -Feat: capitalised type -feat (spawn): space before the scope -EOF +@test "the table agrees with git-cliff itself when git-cliff is installed" { + # The strongest control: the same subjects as commits in a fixture repo, the + # real cliff.toml, and git-cliff's own --unreleased output. Skipped, visibly, + # where git-cliff is absent (CI runners do not ship it). + command -v git-cliff >/dev/null 2>&1 || skip "git-cliff not installed; the table is the measured record" + local d="$BATS_TEST_TMPDIR/cliffrepo" cfg="$BATS_TEST_TMPDIR/cliff-ids.toml" exp s sha kept n=0 + # The real [git] section, verbatim -- the rules are used, not copied -- under + # a template that prints commit ids, because the real template renders the + # description with the type stripped and capitalised, which is not + # comparable to a subject. + { + printf '[changelog]\nbody = """\n{%% for commit in commits %%}{{ commit.id }}\n{%% endfor %%}"""\n' + awk '/^\[git\]/{f=1} f' "$REPO_ROOT/cliff.toml" + } > "$cfg" + grep -q '^commit_parsers' "$cfg" || { echo "the [git] section did not carry over"; return 1; } + git init -q "$d" + git -C "$d" -c user.name=t -c user.email=t@x commit -q --allow-empty -m 'chore: init' + git -C "$d" tag v0.0.1 + : > "$BATS_TEST_TMPDIR/rows" + while IFS='|' read -r exp s; do + git -C "$d" -c user.name=t -c user.email=t@x commit -q --allow-empty -m "$s" + printf '%s|%s|%s\n' "$(git -C "$d" rev-parse HEAD)" "$exp" "$s" >> "$BATS_TEST_TMPDIR/rows" + done < <(_subject_table) + kept="$(cd "$d" && git-cliff --config "$cfg" --unreleased --strip all 2>/dev/null | grep -E '^[0-9a-f]{40}$')" + [ -n "$kept" ] || { echo "git-cliff listed nothing; the fixture or the template is broken"; return 1; } + while IFS='|' read -r sha exp s; do + n=$((n + 1)) + if grep -qx "$sha" <<<"$kept"; then + [ "$exp" = keep ] || { echo "git-cliff KEPT a subject the table says drop: $s"; return 1; } + else + [ "$exp" = drop ] || { echo "git-cliff DROPPED a subject the table says keep: $s"; return 1; } + fi + done < "$BATS_TEST_TMPDIR/rows" + [ "$n" -eq "$(_subject_table | wc -l | tr -d ' ')" ] +} + +@test "first match wins: a skip rule before a keep rule drops, and the reverse keeps" { + # Differential pair on one subject; only the order of the two rules differs. + export AGMSG_CLIFF_CONFIG + AGMSG_CLIFF_CONFIG="$(_cliff_fixture 'filter_unconventional = false' 'skip:^chore' 'keep:(?i)native windows' 'skip:.*')" + run bash "$CHECK" --subject 'chore: native windows bits' + [ "$status" -eq 1 ] + grep -q 'dropped by parser 1' <<<"$output" + AGMSG_CLIFF_CONFIG="$(_cliff_fixture 'filter_unconventional = false' 'keep:(?i)native windows' 'skip:^chore' 'skip:.*')" + run bash "$CHECK" --subject 'chore: native windows bits' + [ "$status" -eq 0 ] + grep -q 'kept by parser 1' <<<"$output" +} + +@test "filter_unconventional absent means git-cliff's default, which drops a non-Conventional subject before any rule" { + # Measured: with the key absent, a subject that matches a keep rule is still + # dropped unless it has the : shape. Differential pair: same rules, + # same subject, only the key differs. + export AGMSG_CLIFF_CONFIG + AGMSG_CLIFF_CONFIG="$(_cliff_fixture '' 'keep:(?i)native windows' 'skip:.*')" + run bash "$CHECK" --subject 'Add native Windows support' + [ "$status" -eq 1 ] + grep -q 'filter_unconventional=true' <<<"$output" + AGMSG_CLIFF_CONFIG="$(_cliff_fixture 'filter_unconventional = false' 'keep:(?i)native windows' 'skip:.*')" + run bash "$CHECK" --subject 'Add native Windows support' + [ "$status" -eq 0 ] +} + +@test "the checker judges the subject line only (a body that would rescue it is out of scope, and says so)" { + # Measured divergence, pinned so a change is deliberate: git-cliff also + # searches the body with unanchored rules. The checker reads one line. + grep -q 'judges the SUBJECT' "$CHECK" + run bash "$CHECK" --subject 'Body rescue test subject' + [ "$status" -eq 1 ] } -@test "the type set is derived from cliff.toml, not retyped: a new type is accepted without editing the script" { - # Differential pair on the same subject; only the fixture differs. +# --- the derivation --------------------------------------------------------------- + +@test "the rules are derived from cliff.toml, not retyped: a rule added there is honoured without editing the script" { export AGMSG_CLIFF_CONFIG - AGMSG_CLIFF_CONFIG="$(_cliff_fixture '^feat' '^fix')" + AGMSG_CLIFF_CONFIG="$(_cliff_fixture 'filter_unconventional = false' 'keep:^feat' 'skip:.*')" run bash "$CHECK" --subject 'wibble: a subject of a type nobody has yet' [ "$status" -eq 1 ] - AGMSG_CLIFF_CONFIG="$(_cliff_fixture '^feat' '^fix' '^(wibble|wobble)')" + AGMSG_CLIFF_CONFIG="$(_cliff_fixture 'filter_unconventional = false' 'keep:^feat' 'keep:^(wibble|wobble)' 'skip:.*')" run bash "$CHECK" --subject 'wibble: a subject of a type nobody has yet' [ "$status" -eq 0 ] } -@test "the derivation reads the real cliff.toml the same way an independent scrape does" { - # Canary for the sed in the script: a second, cruder scrape of the same file - # must agree, and both must find a type that lives only in an alternation - # (`chore`) and one that is skipped rather than grouped (`release`). +@test "--parsers agrees with an independent scrape of the real cliff.toml, in order and in kind" { + # Canary for the parser in the script: a second, cruder scrape of the same + # file must produce the same list. Both must see a skip rule, a keep rule, + # an unanchored legacy rule and the trailing catch-all. local expected got - expected="$(grep -oE 'message *= *"\^[a-z(][a-z|)]*' "$REPO_ROOT/cliff.toml" \ - | sed -E 's/.*"\^//; s/[()]//g' | tr '|' '\n' | sort -u)" - got="$(bash "$CHECK" --subject 'not: a real one' 2>&1 >/dev/null | grep -A1 'derived from cliff.toml' | tail -1 | tr ' ' '\n' | sed '/^$/d' | sort -u)" - [ "$got" = "$expected" ] - grep -qx chore <<<"$got" - grep -qx release <<<"$got" + expected="$(awk '/^commit_parsers *= *\[/{f=1; next} f && /^\]/{exit} f' "$REPO_ROOT/cliff.toml" \ + | grep -oE 'message *= *"[^"]*".*' \ + | awk -F'"' '{ kind = ($0 ~ /skip *= *true/) ? "skip" : "keep"; printf "%d\t%s\t%s\n", NR, kind, $2 }')" + got="$(bash "$CHECK" --parsers | tail -n +2)" + [ "$got" = "$expected" ] || { echo "script:"; echo "$got"; echo "scrape:"; echo "$expected"; return 1; } + grep -qE $'\tskip\t\\^\\(chore' <<<"$got" + grep -qE $'\tkeep\t\\^feat$' <<<"$got" + grep -qF $'\tkeep\t(?i)native windows' <<<"$got" + [ "$(tail -1 <<<"$got")" = "$(printf '%s\tskip\t.*' "$(wc -l <<<"$got" | tr -d ' ')")" ] + [ "$(bash "$CHECK" --parsers | head -1)" = 'filter_unconventional=false' ] } # --- the selection: which subject would land ------------------------------------ @test "a one-commit PR is judged by its commit subject, even when the PR title is fine" { # The asymmetric case that a title-only check gets wrong: #1043 landed - # prefix-less under a fine-looking review, because the commit is what lands. + # dropped under a fine-looking review, because the commit is what lands. local sha sha="$(_repo_with_commit 'Reduce repeated ciphertext literals in pull apply')" run bash "$CHECK" --title 'fix(sync): reduce repeated ciphertext literals' --commits 1 --head "$sha" --repo "$BATS_TEST_TMPDIR/repo" @@ -144,15 +235,17 @@ EOF grep -q 'the PR title' <<<"$output" } -@test "the verdict names what it checked, in both directions" { +@test "the verdict names what it checked and which rule decided, in both directions" { # A green that does not say which subject it read is indistinguishable from # a green that read nothing. run bash "$CHECK" --subject 'fix: something' [ "$status" -eq 0 ] - grep -q 'checked the subject given on the command line' <<<"$output" + grep -q 'Checked the subject given on the command line' <<<"$output" + grep -q 'kept by parser' <<<"$output" run bash "$CHECK" --subject 'something' [ "$status" -eq 1 ] grep -q '^Checked: the subject given on the command line' <<<"$output" + grep -q 'dropped by parser' <<<"$output" } # --- zero targets are not a pass -------------------------------------------------- @@ -163,6 +256,27 @@ EOF grep -q 'nothing to check is not a pass' <<<"$output" } +@test "an option without its value is exit 2 and does not loop" { + # `shift 2` with one argument left fails WITHOUT shifting; with no errexit + # the loop would spin on the same argument forever. Bounded wait, so a + # regression here is a red, not a hung suite. + local pid i + bash "$CHECK" --title >"$BATS_TEST_TMPDIR/out" 2>&1 & + pid=$! + for i in 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20; do + kill -0 "$pid" 2>/dev/null || break + sleep 0.25 + done + if kill -0 "$pid" 2>/dev/null; then + kill "$pid" + echo "still running after 5s: the option loop did not terminate" + return 1 + fi + wait "$pid" && return 1 + [ $? -eq 2 ] + grep -q 'needs a value' "$BATS_TEST_TMPDIR/out" +} + @test "a missing or non-numeric commit count is exit 2" { run bash "$CHECK" --title 'fix: fine' --head deadbeef [ "$status" -eq 2 ] @@ -188,17 +302,20 @@ EOF } @test "a cliff.toml the derivation cannot read is exit 2 even for a good subject" { - # Differential pair on one good subject: only the config differs. The first - # has no parsers at all; the second has parsers but not the canaries. + # Differential set on one good subject: only the config differs. Missing + # file; parsers but no keep rule; parsers but no skip rule; then a usable one. export AGMSG_CLIFF_CONFIG AGMSG_CLIFF_CONFIG="$BATS_TEST_TMPDIR/missing.toml" run bash "$CHECK" --subject 'feat: fine' [ "$status" -eq 2 ] - AGMSG_CLIFF_CONFIG="$(_cliff_fixture '^(chore|ci)')" + AGMSG_CLIFF_CONFIG="$(_cliff_fixture '' 'skip:^chore' 'skip:.*')" + run bash "$CHECK" --subject 'feat: fine' + [ "$status" -eq 2 ] + grep -q 'no usable commit_parsers' <<<"$output" + AGMSG_CLIFF_CONFIG="$(_cliff_fixture '' 'keep:^feat')" run bash "$CHECK" --subject 'feat: fine' [ "$status" -eq 2 ] - grep -q "do not include 'feat'" <<<"$output" - AGMSG_CLIFF_CONFIG="$(_cliff_fixture '^feat' '^fix')" + AGMSG_CLIFF_CONFIG="$(_cliff_fixture '' 'keep:^feat' 'skip:.*')" run bash "$CHECK" --subject 'feat: fine' [ "$status" -eq 0 ] } From d81a99bf96001210d90d9a5a4f904ef0f44b26d8 Mon Sep 17 00:00:00 2001 From: fujibee Date: Mon, 7 Sep 2026 02:41:50 -0700 Subject: [PATCH 3/4] fix(ci): pass a subject a skip rule excludes on purpose, and model breaking 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. --- .github/scripts/check-squash-subject.sh | 143 ++++++++++++++----- .github/workflows/squash-subject.yml | 16 ++- tests/test_check_squash_subject.bats | 177 ++++++++++++++++++------ 3 files changed, 252 insertions(+), 84 deletions(-) diff --git a/.github/scripts/check-squash-subject.sh b/.github/scripts/check-squash-subject.sh index c4596f727..871aec738 100755 --- a/.github/scripts/check-squash-subject.sh +++ b/.github/scripts/check-squash-subject.sh @@ -1,12 +1,22 @@ #!/usr/bin/env bash # -# Fail when the subject a squash merge WILL write is one git-cliff would drop. +# Fail when the subject a squash merge WILL write matches no cliff.toml rule. # -# This checker decides KEEP or DROP -- what git-cliff does with the subject -# under cliff.toml. It does not bind new subjects to the Conventional shape, -# and the two must not be mixed: a Conventional subject can be dropped -# (`chore:` hits a skip rule) and a non-Conventional one can be kept (the -# legacy exceptions in cliff.toml), and a check on the shape gets both wrong. +# This checker rejects a subject that matches none of cliff.toml's commit +# parsers. Whether the matching rule keeps or skips is not the question: +# a skip rule is a deliberate exclusion (`chore:`, `ci:`, `release:` do not +# belong in the notes, and cliff.toml says so), so it passes. What fails is +# the subject nobody decided about -- it falls to the trailing catch-all and +# is dropped without anyone having chosen that. Three answers, then: +# +# matches a keep rule -> exit 0, it will be in the notes +# matches a skip rule -> exit 0, it is left out on purpose +# matches only the catch-all -> exit 1, it is lost by accident +# +# The checker does not bind new subjects to the Conventional shape either: +# a Conventional subject can be excluded (`chore:`) and a non-Conventional +# one kept (the legacy rules in cliff.toml), so a check on the shape answers +# a different question. # # The repository squashes every PR (allow_merge_commit=false, # allow_rebase_merge=false) with squash_merge_commit_title=COMMIT_OR_PR_TITLE. @@ -35,9 +45,34 @@ # LINE only, because that line is what the notes show and what a reviewer # reads; the divergence is confined to cliff.toml's unanchored legacy # rules, and it errs towards red; -# - with `filter_unconventional` absent, git-cliff's default drops every -# non-Conventional commit before the parsers run; this cliff.toml sets it -# to false explicitly, and the script honours whichever it finds. +# - three [git] keys change the answer, and each is read from cliff.toml +# with git-cliff's own default (measured) used only when the key is +# absent -- and the output says which were defaulted: +# conventional_commits (default true) breaking marks are only parsed +# on Conventional subjects +# filter_unconventional (default true) drops every non-Conventional +# commit before the rules run; +# this cliff.toml sets it false, +# which is what lets its legacy +# non-Conventional keep rules +# be reached at all +# protect_breaking_commits (default false) a breaking commit is kept +# even when a skip rule -- or +# the catch-all -- matches it +# first: `chore!:`, `release!:` +# and `wip!:` are all kept when +# it is true, dropped when false +# +# This checker reads the subject line only. The body's `BREAKING CHANGE:` +# footer is not visible to it, so a subject that hits a skip rule (or only +# the catch-all) and is breaking only in its body comes back red -- git-cliff +# keeps it, but that cannot be detected here. The blind spot is deliberately +# on the red side, so that it never produces a false green: a false red is +# seen by a person, who fixes one line or waives it; a false green is seen +# by nobody until the notes are missing an entry. 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. +# If such a subject must pass, put the `!` in the subject, or a person decides. # # The rules are DERIVED from cliff.toml at run time, never retyped here; a # rule added there is honoured by the next run. @@ -48,8 +83,9 @@ # a checker that cannot derive its rule has nothing to be green about. # 2. There is a subject to check. No subject, no commit count, an unreadable # head: exit 2, not 0. Scanning nothing is not a pass. -# 3. The first cliff.toml rule matching the subject has a `group`: exit 0. -# A skip rule, or no rule at all: exit 1, naming the rule and the subject. +# 3. The first cliff.toml rule matching the subject is a keep rule or a +# skip rule written for it: exit 0, naming the rule. Only the catch-all +# (a skip rule that matches everything, `.*`), or nothing: exit 1. # # Usage (CI passes the first form; the second is for a pre-PR check by hand): # check-squash-subject.sh --title "" --commits --head [--repo ] @@ -97,12 +133,13 @@ done # Reads the `commit_parsers = [ ... ]` array of the [git] table line by line: # one `{ message = "", group = "..." }` or `{ ..., skip = true }` per # line, as the file is written. No TOML library, so it runs on any python3. -# Also reads `filter_unconventional` (git-cliff's default when absent: true, -# measured) and `conventional_commits`. +# Also reads the three flags above, each with its measured git-cliff default +# and a note of whether the file or the default supplied it. # -# Prints, one per line: \t\t and, first, a -# header line filter_unconventional=. Exits 2 when the file has -# no usable rules. +# Prints, first, one header line +# config: conventional_commits=(file|default) filter_unconventional=... protect_breaking_commits=... +# then one rule per line: \t\t. Exits 2 when the +# file has no usable rules. derive_rules() { python3 - "$1" <<'PY' import re, sys, pathlib @@ -116,8 +153,14 @@ m = re.search(r'^\[git\]\s*$(.*?)(?=^\[|\Z)', text, re.S | re.M) git = m.group(1) if m else '' def flag(name, default): mm = re.search(r'^\s*' + name + r'\s*=\s*(true|false)\b', git, re.M) - return (mm.group(1) == 'true') if mm else default -filter_unconventional = flag('filter_unconventional', True) + if mm: + return (mm.group(1) == 'true'), 'file' + return default, 'default' +# git-cliff's defaults, measured 2.10.1: conventional_commits true, +# filter_unconventional true, protect_breaking_commits false. +flags = [(n, *flag(n, d)) for n, d in (('conventional_commits', True), + ('filter_unconventional', True), + ('protect_breaking_commits', False))] # The parsers array: everything between `commit_parsers = [` and the closing `]` # that starts a line. a = re.search(r'^\s*commit_parsers\s*=\s*\[(.*?)^\s*\]', git, re.S | re.M) @@ -149,7 +192,7 @@ for k, pat in rules: re.compile(pat) except re.error as e: print(f"{p}: cannot compile parser pattern {pat!r}: {e}", file=sys.stderr); sys.exit(2) -print(f"filter_unconventional={'true' if filter_unconventional else 'false'}") +print("config: " + " ".join(f"{n}={'true' if v else 'false'}({src})" for n, v, src in flags)) for i, (k, pat) in enumerate(rules, 1): print(f"{i}\t{k}\t{pat}") PY @@ -197,52 +240,82 @@ fi # --- 3. what git-cliff would do with it ------------------------------------------ # -# Prints one line: keep|drop -- the first matching rule in -# file order decides, as measured. The rules travel in the environment: the -# python program itself is what `python3 -` reads from stdin. (A function, not -# an inline `$( ... <<'PY' )`: bash 3.2 mis-parses a heredoc with an unbalanced -# parenthesis inside command substitution.) +# Prints one line: keep|skip|lost -- the first matching rule +# in file order decides, as measured. A skip rule that matches EVERYTHING (the +# trailing `.*`, recognised as any pattern that matches the empty string) is +# the catch-all, not a decision about this subject: landing there is `lost`. +# The rules travel in the environment: the python program itself is what +# `python3 -` reads from stdin. (A function, not an inline `$( ... <<'PY' )`: +# bash 3.2 mis-parses a heredoc with an unbalanced parenthesis inside command +# substitution.) judge_subject() { # $1 = subject; RULES in the environment python3 - "$1" <<'PY' import os, re, sys subject = sys.argv[1] lines = os.environ['RULES'].splitlines() -filter_unconventional = lines[0].split('=', 1)[1] == 'true' +cfg = dict(re.findall(r'(\w+)=(true|false)\(', lines[0])) +conventional = cfg['conventional_commits'] == 'true' +filter_unconventional = cfg['filter_unconventional'] == 'true' +protect_breaking = cfg['protect_breaking_commits'] == 'true' rules = [l.split('\t', 2) for l in lines[1:]] +# The Conventional shape git-cliff parses: ()?!?: . +# Both flags below only mean anything for a Conventional subject (measured: +# with conventional_commits=false a `chore!:` is not protected). +conv = re.match(r'^[A-Za-z][A-Za-z0-9_-]*(\([^()]*\))?(!)?: \S', subject) if conventional else None # git-cliff's `filter_unconventional` drops a non-Conventional commit before -# any parser runs. The shape it parses is ()?!?: . -if filter_unconventional and not re.match(r'^[A-Za-z][A-Za-z0-9_-]*(\([^()]*\))?!?: \S', subject): - print("drop\tcliff.toml has filter_unconventional=true (git-cliff's default when absent), " - "and this subject is not of the form ()?!?: , so it is dropped before any parser runs") +# any parser runs. No rule was written for the subject: a loss, not an exclusion. +if filter_unconventional and not conv: + print("lost\tfilter_unconventional is true, and this subject is not of the form " + "()?!?: , so it is dropped before any parser runs") + sys.exit(0) +# A breaking commit is kept regardless of which rule matches -- skip rules and +# the catch-all included (measured: `chore!:`, `release!:`, `wip!:` all kept). +# Only the subject's `!` is visible here; a footer-only BREAKING CHANGE is the +# documented blind spot and falls through to the rules, i.e. towards red. +if protect_breaking and conv and conv.group(2): + print("keep\ta breaking change (`!` in the subject), which protect_breaking_commits keeps whatever rule matches") sys.exit(0) for i, kind, pat in rules: if re.search(pat, subject): if kind == 'keep': print(f"keep\tkept by parser {i} `{pat}`") + elif re.search(pat, ''): + print(f"lost\tmatches no rule but the catch-all (parser {i} `{pat}`), so it is dropped without anyone having decided that") else: - print(f"drop\tdropped by parser {i} `{pat}` (skip = true)") + print(f"skip\tleft out of the notes on purpose by parser {i} `{pat}` (skip = true)") sys.exit(0) -print("drop\tmatches none of cliff.toml's parsers") +print("lost\tmatches none of cliff.toml's parsers, so it is dropped without anyone having decided that") PY } verdict="$(RULES="$rules" judge_subject "$subject")" decision="${verdict%% *}" reason="${verdict#* }" +config="$(printf '%s\n' "$rules" | head -1)" + case "$decision" in keep) echo "$ME: ok -- git-cliff keeps it ($reason). Checked $subject_source:" echo " $subject" + echo " $config" + exit 0 ;; + skip) + echo "$ME: ok -- git-cliff excludes it deliberately ($reason). Checked $subject_source:" + echo " $subject" + echo " $config" exit 0 ;; - drop) - echo "$ME: this subject would be dropped from the release notes: $reason." >&2 + lost) + echo "$ME: this subject would be lost from the release notes: $reason." >&2 echo >&2 echo " $subject" >&2 + echo " $config" >&2 echo >&2 echo "Checked: $subject_source." >&2 echo "The rules are cliff.toml's commit_parsers, first match wins; \`$0 --parsers\`" >&2 - echo "prints them as derived. Retitle the PR, or for a one-commit PR reword that" >&2 - echo "commit (or add a second commit so the PR title is what lands)." >&2 + echo "prints them as derived. A subject a keep rule matches goes into the notes; one" >&2 + echo "a skip rule matches is left out on purpose and passes too. Retitle the PR, or" >&2 + echo "for a one-commit PR reword that commit (or add a second commit so the PR title" >&2 + echo "is what lands)." >&2 exit 1 ;; *) echo "$ME: internal error: no verdict for the subject." >&2 diff --git a/.github/workflows/squash-subject.yml b/.github/workflows/squash-subject.yml index 6f8ac168c..cb34726ad 100644 --- a/.github/workflows/squash-subject.yml +++ b/.github/workflows/squash-subject.yml @@ -1,12 +1,14 @@ -# The subject a squash merge will write must be one git-cliff keeps. +# The subject a squash merge will write must match a cliff.toml rule. # # This repository squashes every PR with squash_merge_commit_title set to # COMMIT_OR_PR_TITLE: a one-commit PR lands under its commit's subject, a # larger one under the PR title (measured, see the script's header). A subject -# without a prefix cliff.toml classifies is silently absent from the release -# notes; that has happened to a headline feature (1.1.7) and to four more -# landings on the 1.3.0 integration branch. This runs the check on the subject -# that would actually land. +# that matches none of cliff.toml's commit parsers falls to the catch-all and +# is silently absent from the release notes without anyone having decided +# that; it has happened to a headline feature (1.1.7) and to four more +# landings on the 1.3.0 integration branch. A subject a skip rule matches is +# left out on purpose and passes. This runs the check on the subject that +# would actually land. # # `edited` is in the event types on purpose: a PR title is fixed by editing it, # and without `edited` the fix would never be re-checked (nor would a title @@ -29,7 +31,7 @@ permissions: jobs: check: - name: squash subject has a type cliff.toml keeps + name: squash subject matches a cliff.toml rule runs-on: ubuntu-latest timeout-minutes: 5 steps: @@ -41,7 +43,7 @@ jobs: ref: ${{ github.event.pull_request.head.sha }} fetch-depth: 1 - - name: The subject that would land has a type cliff.toml keeps + - name: The subject that would land matches a cliff.toml rule (keep or deliberate skip) env: PR_TITLE: ${{ github.event.pull_request.title }} PR_COMMITS: ${{ github.event.pull_request.commits }} diff --git a/tests/test_check_squash_subject.bats b/tests/test_check_squash_subject.bats index f906c8c8d..e3b8e5170 100644 --- a/tests/test_check_squash_subject.bats +++ b/tests/test_check_squash_subject.bats @@ -1,15 +1,18 @@ #!/usr/bin/env bats # # .github/scripts/check-squash-subject.sh: the subject a squash merge will -# write must be one git-cliff KEEPS under cliff.toml, or it is silently absent -# from the release notes. The checker decides keep/drop; it does not bind new -# subjects to the Conventional shape, and this file pins both directions of -# that distinction. Four things are pinned, each with a control the other way: +# write must match a cliff.toml rule, or it falls to the catch-all and is lost +# from the release notes without anyone having decided that. Three answers: +# a keep rule (green, in the notes), a skip rule (green, excluded on purpose), +# only the catch-all (red, lost). The checker does not bind new subjects to +# the Conventional shape, and this file pins all three answers, each with a +# control the other way: # # - the RULE, against what git-cliff itself does (measured, and re-measured # here whenever git-cliff is installed): subjects that already landed and -# were dropped are red, kept ones are green, a skip rule reddens a -# Conventional subject and a legacy group rule greens a non-Conventional one; +# were lost are red; kept ones are green; `chore(ci):` and `release:` hit +# skip rules and are green although git-cliff drops them; a legacy group +# rule greens a non-Conventional subject; the catch-all alone is red; # - the DERIVATION: the rules the script reads out of cliff.toml agree with # an independent scrape, in order and in kind; # - the SELECTION: a one-commit PR is judged by its commit's subject, a larger @@ -25,10 +28,11 @@ setup() { export AGMSG_CLIFF_CONFIG="$REPO_ROOT/cliff.toml" } -# The subjects this file judges, with git-cliff's own verdict for each under -# the real cliff.toml (measured with git-cliff 2.10.1, 2026-09-07). One table, -# read by the rule tests AND by the git-cliff cross-check, so the two cannot -# drift apart. +# The subjects this file judges, with the answer for each under the real +# cliff.toml: `keep` and `skip` are what git-cliff does (measured with +# git-cliff 2.10.1, 2026-09-07 -- `skip` rows are dropped by git-cliff, on +# purpose), `lost` is a subject only the catch-all matches. One table, read by +# the rule tests AND by the git-cliff cross-check, so the two cannot drift. _subject_table() { cat <<'EOF' keep|feat(spawn): set the pane's agent key at spawn so codex seats are not left nameless @@ -38,16 +42,20 @@ keep|feat!: drop the legacy index keep|Add native Windows support for the launcher keep|Role-to-session affinity: pin the seat keep|feature-flag: kept by the ^feat rule, Conventional or not -drop|Naming is an invariant every entry point re-asserts, and the roster can see it (#1044) -drop|Remove internal handles from the published tree, and check for them by identifier context -drop|spawn: launch codex through the bundled monitor shim, and refuse a bare fallback -drop|Reduce repeated ciphertext literals in pull apply -drop|chore(ci): pin the runner image -drop|release: 1.3.0 -drop|ci: a Conventional subject that a skip rule drops -drop|chore: native windows bits -drop|Feat: capitalised -drop|Something unrelated in the subject +keep|chore!: a breaking change that a skip rule matches first +keep|release!: a breaking release +keep|chore(ci)!: a breaking change with a scope +keep|wip!: a breaking change that only the catch-all matches +skip|chore(ci): pin the runner image +skip|release: 1.3.0 +skip|ci: a Conventional subject that a skip rule excludes on purpose +skip|chore: native windows bits +lost|Naming is an invariant every entry point re-asserts, and the roster can see it (#1044) +lost|Remove internal handles from the published tree, and check for them by identifier context +lost|spawn: launch codex through the bundled monitor shim, and refuse a bare fallback +lost|Reduce repeated ciphertext literals in pull apply +lost|Feat: capitalised +lost|Something unrelated in the subject EOF } @@ -82,25 +90,99 @@ _repo_with_commit() { # # --- the rule, both directions ------------------------------------------------------ -@test "every subject in the table gets git-cliff's measured verdict: kept is green, dropped is red" { - local exp s rc +@test "every subject in the table gets its answer: kept is green, skipped on purpose is green, lost is red" { + local exp s rc word while IFS='|' read -r exp s; do run bash "$CHECK" --subject "$s" - case "$exp" in keep) rc=0 ;; drop) rc=1 ;; esac + case "$exp" in keep) rc=0; word='git-cliff keeps it' ;; skip) rc=0; word='excludes it deliberately' ;; lost) rc=1; word='would be lost' ;; esac [ "$status" -eq "$rc" ] || { echo "expected $exp (exit $rc), got exit $status for: $s"; echo "$output"; return 1; } + grep -qF "$word" <<<"$output" || { echo "expected the verdict to say '$word' for: $s"; echo "$output"; return 1; } done < <(_subject_table) } -@test "the table itself has both directions and both distinctions" { - # A table with only one direction would make the test above vacuous. It must - # hold a Conventional subject that is DROPPED (skip rule) and a - # non-Conventional one that is KEPT (legacy group rule). - _subject_table | grep -q '^drop|chore(ci):' - _subject_table | grep -q '^drop|release:' - _subject_table | grep -q '^keep|Add native Windows' - _subject_table | grep -q '^keep|Role-to-session affinity' +@test "the table itself has all three answers and both distinctions" { + # A table missing an answer would make the test above vacuous. It must hold + # a Conventional subject a skip rule EXCLUDES (green although git-cliff + # drops it), a non-Conventional one a legacy group rule KEEPS, and subjects + # only the catch-all matches. + # Fixed strings: a `|` in a basic-regex grep is literal, but say so. + _subject_table | grep -Fq 'skip|chore(ci):' + _subject_table | grep -Fq 'skip|release:' + _subject_table | grep -Fq 'keep|Add native Windows' + _subject_table | grep -Fq 'keep|Role-to-session affinity' + _subject_table | grep -Fq 'keep|chore!:' + _subject_table | grep -Fq 'keep|wip!:' + _subject_table | grep -Fq 'lost|spawn:' [ "$(_subject_table | grep -c '^keep|')" -ge 5 ] - [ "$(_subject_table | grep -c '^drop|')" -ge 5 ] + [ "$(_subject_table | grep -c '^skip|')" -ge 3 ] + [ "$(_subject_table | grep -c '^lost|')" -ge 5 ] +} + +@test "protect_breaking_commits: a breaking subject is kept past a skip rule and past the catch-all, and only when the flag says so" { + # Measured with git-cliff 2.10.1: true keeps `chore!:` and `wip!:`, false + # drops both; absent means false. Differential on one config, one key. + export AGMSG_CLIFF_CONFIG + AGMSG_CLIFF_CONFIG="$(_cliff_fixture $'conventional_commits = true\nfilter_unconventional = false\nprotect_breaking_commits = true' 'skip:^chore' 'keep:^feat' 'skip:.*')" + run bash "$CHECK" --subject 'chore!: breaking past a skip rule' + [ "$status" -eq 0 ] + grep -q 'a breaking change' <<<"$output" + run bash "$CHECK" --subject 'wip!: breaking past the catch-all' + [ "$status" -eq 0 ] + grep -q 'a breaking change' <<<"$output" + AGMSG_CLIFF_CONFIG="$(_cliff_fixture $'conventional_commits = true\nfilter_unconventional = false\nprotect_breaking_commits = false' 'skip:^chore' 'keep:^feat' 'skip:.*')" + run bash "$CHECK" --subject 'chore!: breaking past a skip rule' + [ "$status" -eq 0 ] + grep -q 'excludes it deliberately' <<<"$output" + run bash "$CHECK" --subject 'wip!: breaking past the catch-all' + [ "$status" -eq 1 ] +} + +@test "a flag absent from cliff.toml takes git-cliff's default, and the output says it was defaulted" { + # Measured defaults: conventional_commits true, filter_unconventional true, + # protect_breaking_commits false. A default that is used silently is a + # model nobody can check against the file. + export AGMSG_CLIFF_CONFIG + AGMSG_CLIFF_CONFIG="$(_cliff_fixture 'filter_unconventional = false' 'skip:^chore' 'keep:^feat' 'skip:.*')" + run bash "$CHECK" --subject 'wip!: breaking, but the flag is absent' + [ "$status" -eq 1 ] + grep -q 'protect_breaking_commits=false(default)' <<<"$output" + grep -q 'conventional_commits=true(default)' <<<"$output" + grep -q 'filter_unconventional=false(file)' <<<"$output" + run bash "$CHECK" --parsers + [ "$status" -eq 0 ] + [ "${lines[0]}" = 'config: conventional_commits=true(default) filter_unconventional=false(file) protect_breaking_commits=false(default)' ] +} + +@test "conventional_commits=false switches breaking protection off, as measured" { + export AGMSG_CLIFF_CONFIG + AGMSG_CLIFF_CONFIG="$(_cliff_fixture $'conventional_commits = false\nfilter_unconventional = false\nprotect_breaking_commits = true' 'skip:^chore' 'keep:^feat' 'skip:.*')" + run bash "$CHECK" --subject 'wip!: not parsed as breaking when Conventional parsing is off' + [ "$status" -eq 1 ] +} + +@test "the footer blind spot is on the red side: a subject whose breaking mark is only in the body is red, and the script says why" { + # Measured: git-cliff keeps `wip: ...` with a BREAKING CHANGE: footer under + # protect_breaking_commits=true. The checker sees the subject only and + # cannot; by decision it reports red rather than guessing green. + grep -q 'BREAKING CHANGE' "$CHECK" + grep -q 'blind spot' "$CHECK" + run bash "$CHECK" --subject 'wip: footer breaking, subject shows nothing' + [ "$status" -eq 1 ] +} + +@test "the catch-all is not a decision: a subject only it matches is red, one an explicit skip rule matches is green" { + # Differential pair on one subject; only one explicit skip rule differs. + # Without this distinction the trailing `.*` skip rule would turn every + # lost subject green. + export AGMSG_CLIFF_CONFIG + AGMSG_CLIFF_CONFIG="$(_cliff_fixture 'filter_unconventional = false' 'keep:^feat' 'skip:.*')" + run bash "$CHECK" --subject 'wip: not decided by anyone' + [ "$status" -eq 1 ] + grep -q 'no rule but the catch-all' <<<"$output" + AGMSG_CLIFF_CONFIG="$(_cliff_fixture 'filter_unconventional = false' 'keep:^feat' 'skip:^wip' 'skip:.*')" + run bash "$CHECK" --subject 'wip: not decided by anyone' + [ "$status" -eq 0 ] + grep -q 'excludes it deliberately' <<<"$output" } @test "the table agrees with git-cliff itself when git-cliff is installed" { @@ -128,24 +210,29 @@ _repo_with_commit() { # done < <(_subject_table) kept="$(cd "$d" && git-cliff --config "$cfg" --unreleased --strip all 2>/dev/null | grep -E '^[0-9a-f]{40}$')" [ -n "$kept" ] || { echo "git-cliff listed nothing; the fixture or the template is broken"; return 1; } + # git-cliff lists exactly the `keep` rows; `skip` and `lost` are both absent + # from its output -- the difference between them is whether cliff.toml + # decided it, which is what the checker adds. while IFS='|' read -r sha exp s; do n=$((n + 1)) if grep -qx "$sha" <<<"$kept"; then - [ "$exp" = keep ] || { echo "git-cliff KEPT a subject the table says drop: $s"; return 1; } + [ "$exp" = keep ] || { echo "git-cliff KEPT a subject the table says $exp: $s"; return 1; } else - [ "$exp" = drop ] || { echo "git-cliff DROPPED a subject the table says keep: $s"; return 1; } + [ "$exp" != keep ] || { echo "git-cliff DROPPED a subject the table says keep: $s"; return 1; } fi done < "$BATS_TEST_TMPDIR/rows" [ "$n" -eq "$(_subject_table | wc -l | tr -d ' ')" ] } -@test "first match wins: a skip rule before a keep rule drops, and the reverse keeps" { +@test "first match wins: a skip rule before a keep rule excludes, and the reverse keeps" { # Differential pair on one subject; only the order of the two rules differs. + # Both are green, and the verdict must name which rule decided -- a + # first-match bug would show up as the wrong rule number. export AGMSG_CLIFF_CONFIG AGMSG_CLIFF_CONFIG="$(_cliff_fixture 'filter_unconventional = false' 'skip:^chore' 'keep:(?i)native windows' 'skip:.*')" run bash "$CHECK" --subject 'chore: native windows bits' - [ "$status" -eq 1 ] - grep -q 'dropped by parser 1' <<<"$output" + [ "$status" -eq 0 ] + grep -q 'on purpose by parser 1' <<<"$output" AGMSG_CLIFF_CONFIG="$(_cliff_fixture 'filter_unconventional = false' 'keep:(?i)native windows' 'skip:^chore' 'skip:.*')" run bash "$CHECK" --subject 'chore: native windows bits' [ "$status" -eq 0 ] @@ -160,7 +247,8 @@ _repo_with_commit() { # AGMSG_CLIFF_CONFIG="$(_cliff_fixture '' 'keep:(?i)native windows' 'skip:.*')" run bash "$CHECK" --subject 'Add native Windows support' [ "$status" -eq 1 ] - grep -q 'filter_unconventional=true' <<<"$output" + grep -q 'filter_unconventional is true' <<<"$output" + grep -q 'filter_unconventional=true(default)' <<<"$output" AGMSG_CLIFF_CONFIG="$(_cliff_fixture 'filter_unconventional = false' 'keep:(?i)native windows' 'skip:.*')" run bash "$CHECK" --subject 'Add native Windows support' [ "$status" -eq 0 ] @@ -200,7 +288,8 @@ _repo_with_commit() { # grep -qE $'\tkeep\t\\^feat$' <<<"$got" grep -qF $'\tkeep\t(?i)native windows' <<<"$got" [ "$(tail -1 <<<"$got")" = "$(printf '%s\tskip\t.*' "$(wc -l <<<"$got" | tr -d ' ')")" ] - [ "$(bash "$CHECK" --parsers | head -1)" = 'filter_unconventional=false' ] + # The three flags as the real file sets them, each marked as read from it. + [ "$(bash "$CHECK" --parsers | head -1)" = 'config: conventional_commits=true(file) filter_unconventional=false(file) protect_breaking_commits=true(file)' ] } # --- the selection: which subject would land ------------------------------------ @@ -235,17 +324,21 @@ _repo_with_commit() { # grep -q 'the PR title' <<<"$output" } -@test "the verdict names what it checked and which rule decided, in both directions" { +@test "the verdict names what it checked and which rule decided, in all three answers" { # A green that does not say which subject it read is indistinguishable from - # a green that read nothing. + # a green that read nothing; and the two greens must say which they are. run bash "$CHECK" --subject 'fix: something' [ "$status" -eq 0 ] grep -q 'Checked the subject given on the command line' <<<"$output" grep -q 'kept by parser' <<<"$output" + run bash "$CHECK" --subject 'chore: something' + [ "$status" -eq 0 ] + grep -q 'Checked the subject given on the command line' <<<"$output" + grep -q 'on purpose by parser' <<<"$output" run bash "$CHECK" --subject 'something' [ "$status" -eq 1 ] grep -q '^Checked: the subject given on the command line' <<<"$output" - grep -q 'dropped by parser' <<<"$output" + grep -q 'no rule but the catch-all' <<<"$output" } # --- zero targets are not a pass -------------------------------------------------- From 897bf29861ad83091c29e715277cdcf3999ca2fd Mon Sep 17 00:00:00 2001 From: fujibee Date: Mon, 7 Sep 2026 02:49:03 -0700 Subject: [PATCH 4/4] docs(ci): the footer blind spot changes only the catch-all answer 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. --- .github/scripts/check-squash-subject.sh | 22 +++++++++++++--------- tests/test_check_squash_subject.bats | 6 +++++- 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/.github/scripts/check-squash-subject.sh b/.github/scripts/check-squash-subject.sh index 871aec738..ba9f68c4f 100755 --- a/.github/scripts/check-squash-subject.sh +++ b/.github/scripts/check-squash-subject.sh @@ -64,15 +64,19 @@ # it is true, dropped when false # # This checker reads the subject line only. The body's `BREAKING CHANGE:` -# footer is not visible to it, so a subject that hits a skip rule (or only -# the catch-all) and is breaking only in its body comes back red -- git-cliff -# keeps it, but that cannot be detected here. The blind spot is deliberately -# on the red side, so that it never produces a false green: a false red is -# seen by a person, who fixes one line or waives it; a false green is seen -# by nobody until the notes are missing an entry. 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. -# If such a subject must pass, put the `!` in the subject, or a person decides. +# footer is not visible to it. That changes the answer in exactly one case: +# a subject that matches only the catch-all and is breaking only in its body +# comes back red -- git-cliff keeps it (measured), but that cannot be +# detected here. A subject a skip rule matches is green whether or not its +# body is breaking (the three answers above do not depend on the footer); +# only the stated reason may differ from what git-cliff does. The blind spot +# is deliberately on the red side, so that it never produces a false green: +# a false red is seen by a person, who fixes one line or waives it; a false +# green is seen by nobody until the notes are missing an entry. 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. If such a subject must pass, put the `!` in the subject, +# or a person decides. # # The rules are DERIVED from cliff.toml at run time, never retyped here; a # rule added there is honoured by the next run. diff --git a/tests/test_check_squash_subject.bats b/tests/test_check_squash_subject.bats index e3b8e5170..4635fee8f 100644 --- a/tests/test_check_squash_subject.bats +++ b/tests/test_check_squash_subject.bats @@ -163,11 +163,15 @@ _repo_with_commit() { # @test "the footer blind spot is on the red side: a subject whose breaking mark is only in the body is red, and the script says why" { # Measured: git-cliff keeps `wip: ...` with a BREAKING CHANGE: footer under # protect_breaking_commits=true. The checker sees the subject only and - # cannot; by decision it reports red rather than guessing green. + # cannot; by decision it reports red rather than guessing green. The blind + # spot changes the answer only for a subject that matches nothing but the + # catch-all: a skip-rule subject is green with or without a footer. grep -q 'BREAKING CHANGE' "$CHECK" grep -q 'blind spot' "$CHECK" run bash "$CHECK" --subject 'wip: footer breaking, subject shows nothing' [ "$status" -eq 1 ] + run bash "$CHECK" --subject 'chore: footer breaking, but a skip rule matches' + [ "$status" -eq 0 ] } @test "the catch-all is not a decision: a subject only it matches is red, one an explicit skip rule matches is green" {