Repository navigation
Conversation
Conventional Commits are already the practice here -- 81 of the last 120
commits on main satisfy `cz check` -- but nothing enforces them and nothing
helps an author write one. This wires up commitizen for both.
What it adds:
- `[tool.commitizen]` in pyproject.toml, configured to match how this
repository actually releases: tags are bare semver without a `v` prefix, and
the version lives in those tags because release.yml is what writes them.
- The upstream commitizen hook at the `commit-msg` stage, so `git commit`
checks the message locally.
- A `Commit Message` workflow that checks the pull request title.
- `commitizen` in requirements-dev.txt, and a `Commit messages` section in
CONTRIBUTING.md.
Three decisions worth stating, because each had a plausible alternative.
`cz bump` is not wired in. release.yml already owns versioning: it takes a
version as a workflow input, rewrites the `[package]` version in each crate's
Cargo.toml, then commits, tags and publishes. Adding `cz bump` would give the
version two owners, so commitizen is pointed at the tags rather than at a
version stored in config, and no fourth place has to be kept in step.
`cz bump --dry-run` still reports the version the commits imply, which is a
reasonable way to choose release.yml's input.
CI checks the pull request title rather than the individual commits, because
pull requests are squash-merged here -- 59 of the last 60 commits on main end in
`(#NNN)` and none is a merge commit -- so the title is what becomes the commit
message. Requiring every commit on a branch to be well-formed would reject work
in progress without protecting main. If the merge strategy changes, the
`commitizen-branch` hook from the same upstream is what to add.
The check is its own workflow rather than a job in pr.yml because pr.yml carries
`paths-ignore: guard/ts-lib/**`, and the title of a pull request touching only
that directory still lands on main.
Two things that would otherwise fail silently:
`default_install_hook_types` is set. A `commit-msg` hook is not installed by a
bare `pre-commit install`, so without that line the hook would be configured,
reported as skipped, and never check anything. Contributors who installed hooks
before this change need to run `pre-commit install` again, which CONTRIBUTING.md
says.
The pull request title reaches the check through the environment rather than
`${{ }}` interpolation. A title is author-controlled text and interpolation is
substituted before the shell parses the line.
`changelog_start_rev` is set to the current release. Of the 120 commits before
this change 39 do not satisfy `cz check` -- older bracket-style subjects such as
`[BUG] Update Toolchain`, and dependabot's `Bump <dep> from x to y` -- so a
changelog generated across that boundary would omit the earlier half rather
than describe it.
Verified: a non-conventional message is rejected by the commitizen hook at exit
code 14 and a conventional one commits; `pre-commit install` with no flags lands
both hook types; `cz check` works in a shallow clone with no tags, which is what
CI checks out; the types named in CONTRIBUTING.md are exactly the twelve the
configured pattern accepts, each confirmed against `cz check`; and `pre-commit
run` output is unchanged, which pre-commit.yml greps for specific exit codes.
…ailures `Pre-Commit Hook CI / run-unit-tests-and-lint` was failing on `ruff check ./pre_commit_hooks_tests/ ./pre_commit_hooks` with 6 errors. None of them came from this branch's change: requirements-dev.txt pinned `ruff` only by name, so CI installed whichever release was current, and a newer default rule set started reporting code that had been in the tree untouched. The cap alone does not fix it. `ruff>=0.16.4,<0.17` still resolves to 0.16.5, the current release, so the cap only stops the next minor from moving the goalposts again; the reported code still has to change. Two of the six wanted syntax this hook cannot use. setup.cfg declares `python_requires = >=3.8`, and ruff infers its floor from `project.requires-python`, which this repository does not have. Assuming a modern interpreter, it asked for `Sequence[str] | None` (UP007) and for `Sequence` from `collections.abc` (UP035). Applying either as-is would have broken the hook at its own declared minimum: PEP 604 unions raise `TypeError: unsupported operand type(s) for |` when evaluated before 3.10, and `collections.abc.Sequence` cannot be subscripted before 3.9. `--fix` would have made exactly that change, and CI would have passed anyway -- it runs `python-version: '3.x'`, which resolves to an interpreter where both forms are valid. The break would only have appeared for users on 3.8 or 3.9, at import time. So `[tool.ruff] target-version = "py38"` now states the floor ruff could not infer. That is a fix for the class rather than these two instances: version-gated rules stop proposing syntax the hook cannot run, and they resume on their own if the floor moves. It is load-bearing -- removing it makes UP035 fire again. With the floor stated, ruff asked for `from __future__ import annotations` (FA100) instead, which is the change that actually makes PEP 604 safe here by keeping annotations unevaluated. Added, and the union rewrite applied on top of it. The remaining four were version-independent: import sorting in both modules (I001, applied by ruff), and two nested `with` statements (SIM117). The latter use the comma form rather than parenthesised context managers, which are 3.9+. Verified on Python 3.9.25, below the 3.10 boundary the UP007 rewrite would have crossed, with the versions requirements-dev.txt now resolves to: ruff 0.16.5 clean, mypy 1.19.1 clean, black 25.11.0 unchanged, pytest 8.4.2 4 passed. Separately confirmed the future import is what carries the annotation, by evaluating `Sequence[str] | None` with and without it on 3.9 -- TypeError without, fine with. Not addressed here, since it is a behaviour change rather than a lint fix: `tar.extractfile(member)` is typed `IO[bytes] | None` and returns None for members that are neither regular files nor links, while the loop above it only skips directories.
satyakigh
left a comment
satyakigh
left a comment
There was a problem hiding this comment.
The new workflow does not fully enforce Conventional Commit titles. Commitizen automatically accepts titles beginning with Merge, Revert, Pull request, fixup!, squash!, or amend! without validating their format. The --allow-abort option also causes a title such as # invalid title to pass.
Since the PR title becomes the squash commit on main, these bypasses allow non-conventional commits to be merged. Think cz check --commit-msg-file "$RUNNER_TEMP/pr-title.txt" --allowed-prefixes should work instead
Also need to pin commitizen==4.10.1 in requirements-dev.txt. The pre-commit hook uses 4.10.1, but CI currently installs the latest version, so local and CI validation behavior may differ
`cz check` skipped the Conventional Commits regex entirely for a title beginning with one of its default allowed prefixes (Merge, Revert, Pull request, fixup!, squash!, amend!), and `--allow-abort` accepted an empty message. So both `Merge pull request #1 from nonsense` and `# invalid title` exited 0. The second leaks through the abort path rather than the prefix list: git comment lines are stripped before the regex runs, which leaves an empty message for `--allow-abort` to permit. Since the pull request title becomes the squash commit on main, either one lets a non-conventional commit land there. Passing `--allowed-prefixes` with no values empties the exempt set, and dropping `--allow-abort` closes the empty-message path. All seven default-prefix and comment-line titles now exit 14 (`cz check` uses 14, not 1), while valid conventional titles still exit 0. Pin commitizen to 4.10.1 so that `pip install -r requirements-dev.txt` in CI installs the same version as `rev: v4.10.1` in `.pre-commit-config.yaml`. CI took whatever was newest, so a behavior change between releases could have made the pre-commit hook and the workflow disagree about the same title.
|
Both correct, and both fixed in The bypasses. Measured on commitizen 4.10.1, exit codes captured directly rather than through a pipe. Every one of the documented prefixes passed under the old invocation:
Your The last four rows are there because a check that rejects everything would also have passed the first seven. The valid titles, including this PR's own, still exit 0. The pin. Rebased onto the branch's current head first, so the One thing I did not do, and it is your call. I also left |
…stop reverting each other PRs aws-cloudformation#732 and aws-cloudformation#733 are siblings off main, not a stack: e320695 is not an ancestor of b1e4da3. Both fix the same ruff failure in pre_commit_hooks/cfn_guard.py, and they disagreed on the import spelling, so whichever merged second would have silently reverted the other. aws-cloudformation#732 from typing import Sequence aws-cloudformation#733 from collections.abc import Sequence Take aws-cloudformation#733's spelling. aws-cloudformation#732's rationale for `typing` was that [tool.ruff] target-version = "py38" gates UP035, which is true, but it does not buy anything at the declared floor. Measured on cpython-3.8.20 rather than reasoned about: both spellings import cleanly, and both fail typing.get_type_hints() on main(). The failure reasons differ -- `typing` dies on the PEP 604 `|` ("unsupported operand type(s) for |"), `collections.abc` dies one step earlier on the PEP 585 subscript ("'ABCMeta' object is not subscriptable") -- but neither annotation is evaluable before 3.10 either way, and nothing in pre_commit_hooks/ or pre_commit_hooks_tests/ calls get_type_hints. Under `from __future__ import annotations` the annotation stays a string and main() runs fine on 3.8.20 in both variants. So collections.abc is correct at py38 and at py39+, while `typing` is correct only while the target-version pin holds. pyproject.toml is left exactly as aws-cloudformation#732 has it. target-version = "py38" is now redundant for this file: ruff exits 0 on the collections.abc spelling with the setting present and with it absent. It is still correct, and it still gates the version-sensitive pyupgrade rules for future code, so removing it is a separate call for the maintainer. Measured cells, with a positive control to show the check can fail: collections.abc + target-version = py38 ruff rc=0 collections.abc + no target-version ruff rc=0 typing + no target-version ruff rc=1, UP035 <- control typing + target-version = py38 ruff rc=0 (aws-cloudformation#732 as it stands) pre_commit_hooks/cfn_guard.py is now byte-identical to e320695's copy, blob af1737a on both sides, so the file no longer conflicts between the branches at all. That also picks up aws-cloudformation#733's comment placement in install_cfn_guard(), where the two branches had independently reflowed the same collapsed `with` block.
…ersion This PR is what introduces commitizen, and CONTRIBUTING.md recommended `cz bump --dry-run` as a convenient way to see the version the commits since the last release imply. Measured: it exits 0 and reports `3.1.12 -> 3.2.0`, and 3.2.0 had already been released. A contributor following the file would have taken a version number that is not merely stale but already taken. Cause. `version_provider = "scm"` makes commitizen read the current version from the newest tag that is an ancestor of HEAD. The release tags are not ancestors: `release.yml` tags the commit on the PR branch, and squash-merge lands a different object on main. Tag 3.2.1 is e531ef5, `chore: bump version to 3.2.1` with parent fcc079f; main carries 3e265bb, `chore: bump version to 3.2.1 (aws-cloudformation#730)`, with parent ed7c703. Different SHA, different tree, different parent. The newest version tag reachable from main is 3.1.12. Both halves of the output are wrong, not just the base version. Over the range commitizen actually walked, 3.1.12..HEAD, there are three `feat:` commits, all of which shipped in 3.2.0 or 3.2.1, so it reported a minor increment. Over the range it should have walked, 3.2.1..HEAD, there are no `feat:` commits and the highest type is `fix:`, so the correct answer was a patch: 3.2.2, not 3.2.0. Qualified rather than removed. The command is the obvious thing to try once commitizen is installed, so deleting the line leaves a contributor to find the wrong number with no warning attached. The replacement states the mechanism, so a reader can tell why the number is wrong and recognise when it stops being wrong, which "may be inaccurate" would not give them. The replacement recommends reading the tag directly. It filters to the bare `N.N.N` shape on purpose: this repository also carries `action-v*`, `pre-commit-v*` and older `v`-prefixed tags, and an unfiltered `git tag --sort=-v:refname` puts `v2.1.0-pre-rc1` first. The unfiltered form was in a draft of this paragraph and would have replaced one wrong instruction with another. changelog_start_rev = "3.2.1" is left alone deliberately. It names one of the off-history tags, but it is neither inert nor broken: `cz changelog` renders the correct Unreleased section. It survives because commitizen's range excludes the tagged commit's ancestry, and e531ef5's parent fcc079f *is* on main, so the boundary still falls in the right place. Measured both ways: with the setting, `git log 3.2.1..HEAD` is 6 commits and the changelog shows only the 2 `fix:` entries; without it the Unreleased section would span 60 commits back to 3.1.12. The imprecision is that the squashed release commit 3e265bb falls inside the range rather than bounding it, and that commit is `chore:`, which is not rendered. Worth knowing that this depends on the tagged commit's parent being on main, but there is nothing to fix here today.
The gate is one `cz check` invocation in a workflow file, and the two flags that make it strict are the kind of thing a later edit drops without noticing. Measured against commitizen 4.10.1: with `--allow-abort` and without `--allowed-prefixes`, all of `Merge pull request #1 from nonsense`, `Revert whatever I feel like, unformatted` and `# invalid title` exit 0. With the flags as they now stand, all three exit 14 and a well-formed title still exits 0. Nothing failed when those bypasses were open, so the fix that closed them has no guard. These tests are that guard. Reverting the workflow line to the earlier command fails seven of them, one per bypass, which is how they were checked rather than assumed. The command is read out of commit-message.yml rather than restated, so the assertion follows what CI runs instead of what the test remembers it running -- a duplicated argv would keep passing after the workflow changed, which is the exact failure being guarded against. Accepted titles are asserted alongside rejected ones so a gate broken into refusing everything fails too, and the rejection is pinned to exit 14 specifically so a crash does not read as a correct refusal. The commitizen pin gets its own assertion comparing requirements-dev.txt against the pre-commit hook's rev, because `cz check`'s allowed-prefix list and exit codes are defaults that have moved between releases: unpinned, a local pass stops predicting a CI pass with neither file changing. commit-message.yml and requirements-dev.txt are added to pre-commit.yml's path filters. Both are inputs to these tests, and without them an edit to either would change what the tests are about while leaving them unrun.
|
Both of your points were right, and both are in as of I verified the behavior rather than just the flags, against 4.10.1. With the command as it was when you reviewed (
With the command as it now stands ( What was missing is that nothing failed while those bypasses were open, so the fix had no guard — a flag dropped from that line would silently reopen them, and the title still becomes the squash commit. It reads the The pin gets its own assertion comparing Measured on |
The `Revert` case asserted on `Revert whatever I feel like, unformatted`, a string nothing generates. The bypass that costs anyone something is GitHub's revert button, which proposes `Revert "<original title>"`; that shape now has the case. It fails under the pre-fix command, so it is a reproduced bypass rather than an invented one. Keeping it rejected, with the reason. `--allowed-prefixes Revert` cannot be narrowed to the button's output: commitizen matches with `commit_msg.startswith(prefix)`, so the allowance also exempts `Reverted the thing, no type at all` from the regex. Measured both. Two of 641 commits on main arrived from that button, and none used the conventional `revert:`, which is an accepted type here -- so the cost of rejecting is one retitle, against an unchecked title landing as the squash commit on main. CONTRIBUTING.md now says so where a contributor will hit it, and `revert:` is in the accepted cases so that advice cannot go stale silently. Also correct the module docstring. It claimed three titles passed the pre-fix command; seven of the eight did. It now records the ablation instead: emptying the prefix list closes the six prefix titles and nothing else, dropping `--allow-abort` closes `# invalid title` and nothing else, so both flags are load-bearing.
b8ccf13 removed the recommendation from CONTRIBUTING.md and left it in the `[tool.commitizen]` comment, so the two files disagreed about the same command: one says do not use it to pick a release version, the other called it a reasonable way to choose the input for the release workflow. Both places are now reconciled to the measured behavior -- `cz bump --dry-run` proposed 3.2.0, already released, and read the increment as minor from `feat:` commits that shipped earlier. The comment points at CONTRIBUTING.md for the mechanism rather than restating it. Anyone changing what `cz bump` does here has two files to update, not one; this commit exists because the first attempt updated one.
|
You were right on all three points, and this has been sitting unanswered for two weeks — sorry. The fix landed in The workflow now runs I reproduced all seven bypasses against the old command before changing anything — none was hypothetical. The ablation is worth recording, because it shows both fixes are load-bearing:
Emptying the prefix list closes the six and does nothing for On why On I confirmed the job actually fails rather than just that Two further fixes on this branch from that verification: |
Conventional Commits are already the practice in this repository — 81 of the last 120 commits on
mainsatisfycz check— but nothing enforces them and nothing helps an author write one. This wires up commitizen for both.What this adds
pyproject.toml[tool.commitizen], configured to match how this repository actually releases.pre-commit-config.yamlcommit-msgstage.github/workflows/commit-message.ymlrequirements-dev.txtcommitizenCONTRIBUTING.mdCommit messagessectionThree decisions, each with a plausible alternative
cz bumpis deliberately not wired in.release.ymlalready owns versioning: it takes a version as a workflow input, rewrites the[package]version in each crate'sCargo.toml, then commits, tags and publishes. Addingcz bumpwould give the version two owners. So commitizen is pointed at the git tags (version_provider = "scm") rather than at a version stored in config, and no fourth place has to be kept in step with the threeCargo.tomlfiles.cz bump --dry-runstill reports the version the commits since the last tag imply, which is a reasonable way to choose the input to giverelease.yml.CI checks the pull request title, not the individual commits. Pull requests are squash-merged here — 59 of the last 60 commits on
mainend in(#NNN)and none is a merge commit — so the title is what becomes the commit message. Requiring every commit on a branch to be well-formed would reject work in progress without protectingmain. If the merge strategy ever changes to merge commits or rebase, thecommitizen-branchhook from the same upstream repository is the thing to add, and there is a comment in the workflow saying so.It is its own workflow rather than a job in
pr.yml.pr.ymlcarriespaths-ignore: guard/ts-lib/**, so a pull request touching only that directory does not run it — and the title of such a pull request still lands onmain.Two things that would otherwise fail silently
default_install_hook_typesis set. Acommit-msghook is not installed by a barepre-commit install; only thepre-commitstage is. Without that line the hook would be configured, reported bypre-commit runas skipped, and never check anything. Contributors who installed hooks before this change need to runpre-commit installagain —CONTRIBUTING.mdsays so.The pull request title reaches the check through the environment, not
${{ }}interpolation. A title is author-controlled text, and interpolation is substituted before the shell parses the line, so a title containing shell syntax would otherwise run as part of the step.Why the changelog is not generated here
changelog_start_revis set to the current release. Of the 120 commits before this change, 39 do not satisfycz check— older bracket-style subjects such as[BUG] Update Toolchain, and dependabot'sBump <dep> from x to y. A changelog generated across that boundary would omit the earlier half rather than describe it, so generation starts from3.2.1.Verification
commitizen check.....Failed), and a conventional one commits. Confirmed it is the commitizen hook rejecting it, not the existingcfn-guardhook, which passes in both cases.pre-commit installwith no flags lands both.git/hooks/pre-commitand.git/hooks/commit-msg.cz checkworks in a shallow clone with zero tags, which is what CI checks out — the one real risk in choosingversion_provider = "scm".CONTRIBUTING.mdare exactly the twelve the configured pattern accepts, each confirmed individually againstcz check.pre-commit runandpre-commit run --all-filesoutput is unchanged: commitizen is acommit-msg-stage hook and appears in neither.pre-commit.ymlgreps that output forexit code: 19andexit code: 7, so this matters.cz check.black --checkwas not run locally — the module is not installed in this environment. No.pyfiles are changed, so it is unaffected.