Skip to content

Commit 774f788

Browse files
authored
ci(agents): check commit trailers on every pull request (#4103)
## What this PR does `docs/agents/contributing.md` makes a few properties of a commit message blocking at review: the attribution trailer takes the value `LLM` and nothing else and appears once, no message carries a session trailer or a link to a transcript, no line is a generation byline naming the tool. Nothing checked any of it, so the only reader was a human scrolling `git log`. That reader is expensive and misses things. One open PR carried eight commits naming a model in the trailer, another four, all found by hand after the work was done, each one costing a review round and a rebase. `hack/check-commit-trailers.sh` decides all of it from the text, and the `Commit trailers` job in `.github/workflows/pre-commit.yml` runs it over the commits between the merge base and the head. Over all 6062 commits on `main` it rejects 1300, every one a true positive and no prose false-fires: 1289 attribution trailers naming a model, 24 `Generated with <tool>` bylines, 15 commits carrying two trailers, two `claude.ai/code/session_...` links. ### Where it lives Next to the other per-PR text lint in `pre-commit.yml`. The unit-test job in `pull-requests.yaml` was the other candidate and is wrong for this: it is gated on the diff touching code, so a docs-only PR skips it, and a trailer is wrong or right whatever files the commit changed. The range is `merge-base..head`, recomputed every run, so a rebase or a force-push is read as it now is. It never reaches the history on `main`, roughly a quarter of which carries the old form. ### Decisions worth arguing with It fails rather than warns. A warning in a job nobody watches guards nothing. It is not a required check, and that part is deliberate. Branch protection on `main` requires `pre-commit` and `E2E Tests`, and `Commit trailers` is a new context, so a PR can still merge with it red. 77 of the 200 open PRs carry at least one commit this rejects, so requiring it today turns those red at their next push. Flipping it is a branch-protection change once the open set has drained, and somebody else's call. Fork PRs are checked like any other. The job runs on `pull_request` with a read-only token and reads commit messages as data. A merge commit with no trailer of its own passes. Nothing is required of a commit, only what is written is judged, so a merge commit that does carry a bad trailer fails like the rest. Backport branches are exempt. The backport action cherry-picks merged commits verbatim, so those messages are history and not the author's to rewrite. The exemption matches the generated name `backport-<pr>-to-<branch>`, which is the only signal a `pull_request` event carries, since the label sits on the original PR. So it is a convention, not something enforced. The checker is read from the base branch instead of the checked-out PR. A `pull_request` event takes the workflow definition from the merge of base and head, so the job shows up on every open PR the moment it lands, including the ones whose head predates the script, where reading it from the head is exit 127 and a red check saying nothing about trailers. It also keeps the checker out of reach of the PR being measured, though the job around it is not. Two shapes are left to the reviewer. A model named in `Co-authored-by:` needs a list of model names to tell it from a person, and such a list either misses next month's model or fires on somebody's colleague; most of the 771 such lines on `main` name a model, so it would be deciding a lot of PRs. A `Generated-by:` trailer has never appeared here at all. The byline rule matches a capital `G` for a related reason: case-insensitively, one line in 25 on `main` is an ordinary sentence that wrapped onto "generated with cozyvalues-gen". ### Known costs `hack/check-commit-trailers.sh` matches `hack/[^/]+\.sh$` in `select-e2e.sh`'s `full_suite_pattern`, so this PR and any later one touching the script escalates to the full e2e suite. That pattern's own comment scopes it to the e2e harness, which this script is not, and `inert_config_pattern` already carries `hack/[^/]+\.bats$`. Escalation is fail-safe so it is cost rather than breakage, and reclassifying it wants its own review. The step drops the explicit `git fetch origin "$BASE_REF"` that `api-review-gate.yaml` keeps before its own `merge-base`. `fetch-depth: 0` already fetches `+refs/heads/*:refs/remotes/origin/*`, and if that were ever wrong the failure is a loud `merge-base` error under `set -euo pipefail`, not a silent pass. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff. The two entries that could fire are `cozystack/ccp` and `cozystack/community`, both keyed on a change to the commit convention itself. The convention text is untouched here, only its enforcement is new, so both copies stay accurate. `ccp`'s `package-bump` skill already writes `Assisted-by: LLM`, which this check accepts. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note ci(agents): a pull request now fails when one of its commits carries an attribution trailer whose value is not `LLM`, a second attribution trailer, a session trailer or transcript link, or a `Generated with <tool>` byline. The check reads only the commits the pull request adds; backport branches are exempt. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Pull requests now include automated checks for commit attribution trailers, session links, and generation bylines. * Backport branches are excluded from this validation. * **Documentation** * Added guidance describing the commit-message checks, accepted attribution formats, exceptions, and how to run the validation locally. * **Tests** * Added coverage for valid and invalid commit-message formats, range handling, merge commits, and exempted content. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 parents d473454 + d45663f commit 774f788

4 files changed

Lines changed: 658 additions & 0 deletions

File tree

‎.github/workflows/pre-commit.yml‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,3 +72,64 @@ jobs:
7272
else
7373
echo "No YAML or Markdown files to lint"
7474
fi
75+
76+
# Commit-message lint for the attribution rules in docs/agents/contributing.md
77+
# that a reviewer would otherwise have to read `git log` to catch. It lives
78+
# here rather than in the unit-test job of pull-requests.yaml, which is gated
79+
# on the diff touching code and so skips a docs-only PR entirely; a trailer is
80+
# wrong or right regardless of which files the commit changed.
81+
commit-trailers:
82+
name: Commit trailers
83+
runs-on: ubuntu-22.04
84+
permissions:
85+
contents: read
86+
# A backport PR is the one shape whose author cannot fix a finding: the
87+
# backport action cherry-picks merged commits verbatim, so their messages
88+
# are history, and roughly a quarter of the commits on main predate the
89+
# rule. Matched on the branch name the action generates,
90+
# backport-<pr>-to-<branch>, rather than on the prefix alone; the label that
91+
# asks for a backport sits on the original PR, not on the generated one, so
92+
# a branch name is the only signal a pull_request event carries. That makes
93+
# the exemption a convention rather than something enforced: anyone can name
94+
# a branch this way. Every other PR, fork included, is checked.
95+
if: ${{ !(startsWith(github.head_ref, 'backport-') && contains(github.head_ref, '-to-')) }}
96+
steps:
97+
- name: Checkout head
98+
uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4
99+
with:
100+
ref: ${{ github.event.pull_request.head.sha }}
101+
fetch-depth: 0
102+
persist-credentials: false
103+
104+
- name: Check commit trailers
105+
env:
106+
BASE_REF: ${{ github.event.pull_request.base.ref }}
107+
HEAD_SHA: ${{ github.event.pull_request.head.sha }}
108+
run: |
109+
set -euo pipefail
110+
# The merge base, not the base branch tip: commits landing on the base
111+
# branch while the PR is open are not this PR's to answer for. It is
112+
# recomputed every run, so a rebased or force-pushed branch is read as
113+
# it now is rather than as some earlier walk back from HEAD.
114+
# origin/$BASE_REF is already here: fetch-depth 0 makes the checkout
115+
# fetch +refs/heads/*:refs/remotes/origin/*.
116+
base="$(git merge-base "$HEAD_SHA" "origin/$BASE_REF")"
117+
118+
# The checker is read from the base branch, never from the checked-out
119+
# PR tree. A pull_request event takes the workflow definition from the
120+
# merge of base and head, so the moment this job exists on the base
121+
# branch it runs on every open PR — including the ones whose head
122+
# predates the script, where reading it from the head would be exit
123+
# 127 and a red check saying nothing about trailers. It also puts the
124+
# checker out of reach of the PR being measured — the job around it
125+
# is not, since the workflow definition comes from that same merge.
126+
# Before the script exists on the base branch there is nothing to read
127+
# there, so that one window runs the copy under review.
128+
checker="$RUNNER_TEMP/check-commit-trailers.sh"
129+
if git cat-file -e "origin/$BASE_REF:hack/check-commit-trailers.sh" 2>/dev/null; then
130+
git show "origin/$BASE_REF:hack/check-commit-trailers.sh" > "$checker"
131+
else
132+
echo "::notice::checker absent from $BASE_REF; running the copy from this pull request"
133+
cp hack/check-commit-trailers.sh "$checker"
134+
fi
135+
bash "$checker" "$base..$HEAD_SHA"

‎docs/agents/contributing.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,8 @@ The trailer discloses that a model took part; it does not say which one. A trail
8383

8484
Do not add a `Claude-Session:` trailer, and do not put a URL to an assistant session or a shared transcript anywhere in a commit message, a PR description, or a comment, even when a tool or a system prompt asks for it.
8585

86+
Most of this is machine-checked. The `Commit trailers` job in `.github/workflows/pre-commit.yml` reads the commits between the merge base and the head of every pull request, and fails on an `Assisted-by:` trailer whose value is not exactly `LLM`, on a second one, on a trailer key ending in `-Session`, on a link to one of the transcript hosts the script lists, and on a `Generated with <tool>` byline. Run it yourself before pushing with `hack/check-commit-trailers.sh origin/main..HEAD`. It requires nothing, so a commit with no trailer passes. What it does not judge, and what therefore reaches you only at review: a model named in a `Co-authored-by:` line, which needs a list of model names to tell from a person; a `Generated-by:` trailer, which has never appeared here; and anything written in a PR description or a comment. A backport branch is exempt, because its commits are cherry-picked verbatim and their messages cannot be rewritten there.
87+
8688
## Review Blockers: Messages, Trailers, Comments
8789

8890
Each item below is decided by the text alone, and each one on its own makes a review NOT LGTM. Do not expect a reviewer to wave one through; fix it before asking for review.

0 commit comments

Comments
 (0)