ci: validate commit messages with pre-commit - #18704
ci: validate commit messages with pre-commit#18704Tobias Brick (tobiasb-ms) wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new multi-line workflow step omits the repository-required shell-hardening setup.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds Commitizen-based commit-message validation to local pre-commit hooks and pull-request CI.
Changes:
- Defines Azure Linux Conventional Commit rules.
- Validates complete PR commit ranges in lint CI.
- Removes PR-title validation.
File summaries
| File | Description |
|---|---|
.cz.toml |
Defines commit-message rules. |
.pre-commit-config.yaml |
Adds Commitizen hooks. |
.github/workflows/lint.yaml |
Validates PR commit history. |
.github/workflows/check-pr-title.yml |
Removes title validation. |
CONTRIBUTING.md |
Documents commit checks. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
62db687 to
740b7f1
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The implementation contradicts the PR description and omits required workflow shell hardening.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Balanced
740b7f1 to
0fcbe8e
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new workflow executes PR-controlled hooks directly on the runner host instead of within the required sandbox.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
0fcbe8e to
1d41cec
Compare
|
/azp run |
|
Azure Pipelines: 2 pipeline(s) were filtered out due to trigger conditions. |
There was a problem hiding this comment.
🟢 Approval recommended
The implementation consistently shares the documented Commitizen policy between local hooks and CI without identified defects.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
| hooks: | ||
| - id: commitizen | ||
| args: | ||
| - --allow-abort |
There was a problem hiding this comment.
question(blocking): What do you think about factoring out more of the commitizen config/cmdline options out into the commitizen config file the PR includes? (Is that feasible?)
With editorconfig, we adopted a pattern where there's a separate .editorconfig settings file that encapsulates our desired policy -- and the pre-commit config for it is just responsible for selecting version of the tool and filtering on which inputs to apply it to. That seems like a good separation of concerns. I see this PR also includes a .cz.toml file but leaves allowed_prefixes blank.
There was a problem hiding this comment.
[Perihelion🚀] I think the current split is intentional. .cz.toml defines the strict repository policy used for final branch validation, while .pre-commit-config.yaml defines how Commitizen is invoked at each Git hook stage. --commit-msg-file is commit-msg hook plumbing, --allow-abort permits cancelling a commit, and the prefix override allows temporary fixup!, squash!, and amend! commits during development. commitizen-branch receives none of those overrides, so CI uses .cz.toml’s strict allowed_prefixes = []. Moving the exceptions into .cz.toml would make CI accept them too.
There was a problem hiding this comment.
I'm not sure I follow. If there's a desire for development affordances, then shouldn't the default local dev use to be more permissive? And wouldn't the default .cz.toml affect that and require the allow-listing of these prefixes?
I'm also not convinced there is any need for temporary prefixes.
| `fixup!`, `squash!`, and `amend!` commits are accepted by the local commit-message hook | ||
| to support review workflows, but CI rejects them from the final pull request history. | ||
|
|
||
| To check every commit on your branch before pushing, run: |
There was a problem hiding this comment.
issue(non-blocking): I don't see folks (or AI) remembering to run a command like this. Is it possible for the pre commit config file itself to request running this at pre-push via the stages property? Or was there a reason you didn't want to do that yet?
There was a problem hiding this comment.
[Perihelion🚀] The commitizen-branch hook already declares stages: [pre-push], so developers can install it with pre-commit install --hook-type pre-push and have it run automatically. I intentionally didn’t include pre-push in default_install_hook_types, because our documented review workflow permits pushing temporary fixup!, squash!, and amend! commits; the strict branch hook would block those pushes. CI always runs the branch check, so the command here is only an optional local preflight. I’ve changed the wording to make clear that it is intended for checking the cleaned-up history before requesting approval.
| @@ -0,0 +1,42 @@ | |||
| name: "Check Commit Messages" | |||
There was a problem hiding this comment.
question(blocking): Does this actually have be run separately from the existing lint workflow? Can we not just run pre-commit and have it run all of its checks in one go?
There was a problem hiding this comment.
It can’t run in the same pre-commit invocation: --all-files controls file selection, not hook stages. The existing command runs pre-commit-stage hooks, while commitizen-branch is a pre-push-stage hook and needs an explicit base-to-head revision range. We could invoke it separately from within lint.yaml, but commit-history validation is logically distinct from file linting; pre-commit is only the common runner. I kept it as a separately named workflow so file-content and commit-history validation run and report independently.
I’m happy to discuss consolidating them if you’d prefer a single CI workflow.
Use Commitizen for both local commit-msg checks and CI validation of every commit in a pull request. Allow temporary fixup commits locally while rejecting them from the final branch history. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Commit messages, rather than pull request titles, are the permanent records preserved by the rebase-merge workflow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1d41cec to
aaac7c3
Compare
| - name: Checkout | ||
| uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 | ||
| with: | ||
| fetch-depth: 0 |
There was a problem hiding this comment.
suggestion(non-blocking): In the words of agents everywhere, this fetch-depth is ... load bearing 😄. It's probably worth a comment explaining that it's required to ensure history-based checks; otherwise, the checkout default would be missing history.
| hooks: | ||
| - id: commitizen | ||
| args: | ||
| - --allow-abort |
There was a problem hiding this comment.
I'm not sure I follow. If there's a desire for development affordances, then shouldn't the default local dev use to be more permissive? And wouldn't the default .cz.toml affect that and require the allow-listing of these prefixes?
I'm also not convinced there is any need for temporary prefixes.
Summary
Implementation
The repository's Conventional Commit schema lives in
.cz.toml. It permits the documented Azure Linux commit types, optional scopes, and breaking-change markers while rejecting unsupported types and summaries ending in a period.The pre-commit configuration exposes two Commitizen hooks:
commitizenvalidates each new commit message locallycommitizen-branchvalidates the complete commit range before a push or in CITemporary
fixup!,squash!, andamend!commits are allowed by the localcommit-msghook to support review workflows. The branch-range check rejects them before merge.A dedicated Check Commit Messages workflow checks out the complete pull request history and invokes
commitizen-branchusing the pull request's base and head SHAs. The workflow contains no commit-message policy of its own: the pinned hook and rules remain in.pre-commit-config.yamland.cz.toml, shared with local development.Keeping commit-history validation separate from the existing Lint workflow avoids coupling it to Go setup,
azldevinstallation, TOML validation, or file-oriented pre-commit checks.Because Azure Linux uses rebase-merge and preserves individual commits, the existing pull-request-title validation workflow is removed.
Testing
Fixes: AB#22437