chore: validate pull request titles - #50
Conversation
apgiorgi
left a comment
apgiorgi
left a comment
There was a problem hiding this comment.
This doesn't do what it claims, and the validation list contradicts our conventions.
1. The premise is wrong for this repo.
We're rebase-merge only — allow_squash_merge: false, allow_merge_commit: false, allow_rebase_merge: true. The PR title never becomes a commit message; individual commits land verbatim. GoReleaser's changelog filters operate on commit subjects (.goreleaser.yaml:73-82), so a validated PR title has zero effect on release notes.
The PR body's rationale — "catches inconsistent squash or rebase titles" — doesn't apply here. If we want to enforce prefixes for changelog purposes, the thing to validate is commit messages, not the title. validateSingleCommit is the only knob in this action that touches them, and it's unused.
2. The allowed-prefix list doesn't match AGENTS.md.
pr-title.yml:17-21 sets no types:, so the action falls back to defaultTypes (src/validatePrTitle.js:7,22 at the pinned SHA): feat, fix, docs, style, refactor, perf, test, build, ci, chore, revert. AGENTS.md documents only docs:, test:, chore: as filtered, plus fix: and feat:.
Consequence: style:, refactor:, perf:, build:, ci:, revert: all pass validation but are not in the .goreleaser.yaml:77-82 exclude list, so they'd leak straight into user-facing release notes. A gate that green-lights prefixes we don't use is worse than no gate. Needs an explicit types: of exactly the five.
3. pull_request_target (pr-title.yml:4) is unnecessary. No checkout, no write permissions, no secrets beyond GITHUB_TOKEN — plain pull_request suffices and matches house style (ci.yml:3). Same precedent concern as #48: a later edit adding actions/checkout of the PR head turns this into fork-controlled code running with the base-repo token.
It also means this workflow never ran on its own PR — pull_request_target loads the workflow from main, and there's no validate-title check in gh pr checks 50, contradicting "GitHub CI on this pull request" in the body.
Nits
- Permissions are otherwise least-privilege;
pull-requests: readis arguably unneeded too. - Action is pinned to v5.5.3 (2024-06-28, node20); current is v6.1.1. No known vuln, just a stale major with no dependabot to bump it.
pr-title.yml:13if: user.type == 'User'— if this ever becomes a required check, bot PRs report skipped. Worth confirming that's intended.requireScope: false(:21) is already the default;synchronize(:5) can't change a title.
Structurally identical to #48 and #51. If we want any of this, one pr-hygiene.yml with three jobs beats three files. And per CONTRIBUTING.md, changes to repo surface like this want an issue first.
|
Closing this after validating the review finding. This repository is rebase-merge-only, so PR titles do not become commit subjects and cannot control GoReleaser changelog entries. The proposed allowed types also did not match the repository’s release filters. A future check, if needed, should validate commits themselves and be scoped as a separate task. |
2 participants
Summary
Jira: https://doitintl.atlassian.net/browse/CMP-48969
Why
dci-cli release notes are generated from commit titles, and the repository already expects conventional prefixes such as
feat:,fix:, andchore:. Validating the PR title catches inconsistent squash or rebase titles before merge.Test methods
git diff --checkAfter merge, open or edit a human-authored PR with a non-conventional title to see the check fail, then change it to a title such as
chore: test title validationto see it pass.Could this break things?
Risk: low. This adds a required-looking check but does not change repository branch protection by itself. Existing open PRs may get validated the next time their title or commits change. Bot-authored PRs are skipped.