mirror of
https://github.com/cozystack/cozystack.git
synced 2026-10-28 23:57:08 +09:00
Andrei Kvapil
774f788016
## 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 -->