feat(ci): force check pr title#6396
Conversation
WareWolf-MoonWall
left a comment
There was a problem hiding this comment.
Review β #6396 feat(ci): force check pr title
Verdict: request-changes
Reviewer: WareWolf-MoonWall
Head checked: (latest at review time β 2 commits, checks passing)
I read the full diff, the linked issue #6394, the existing workflow files in .github/workflows/, and FND-004 Β§5.4 (Action Pinning Policy) and Β§8 (What This Means for Contributors). No prior reviews or inline comments to resolve. The checks gate is green on the non-title-lint jobs, but the new workflow itself is what's under review.
π΄ Blocking β action is not SHA-pinned
Per FND-004 Β§5.4, the policy is unambiguous:
All
uses:references in workflow files must be pinned to a full commit SHA with a version comment. Mutable tags (@v4,@main,@latest) are not permitted. No exceptions.
The PR ships:
- uses: amannn/action-semantic-pull-request@v5@v5 is a mutable tag. If ilteoood/action-semantic-pull-request (or a fork/mirror that hijacks the name) ever pushes a tag update, this workflow runs different code than what was reviewed β with pull-requests: read permission and access to GITHUB_TOKEN. That is a meaningful supply-chain exposure for any workflow file, even one with narrow permissions.
The fix is a single-line change. Look up the current v5 SHA:
git ls-remote https://github.com/amannn/action-semantic-pull-request refs/tags/v5Then pin:
- uses: amannn/action-semantic-pull-request@<full-sha> # v5.x.yThis also needs to be added to whatever dependency-update process keeps the other workflow action SHAs current (Dependabot or Renovate, whichever is in use).
π΄ Blocking β file extension inconsistency
Every other workflow file in .github/workflows/ uses the .yml extension:
ci.yml
daily-audit.yml
docs-deploy.yml
pr-path-labeler.yml
...
The new file is named pr-title.yaml. GitHub Actions accepts both extensions, but this introduces an inconsistency that will create confusion on ls, in search results, and in any script that globs *.yml. Please rename to pr-title.yml to match the established convention.
π‘ Warning β requireScope: true with no scope enumeration
The action is configured with requireScope: true but no scopes: allowlist. That means any string is accepted as a scope as long as one is present. This is workable, but the combination with requireScope: true will gate PRs that have valid conventional-commit titles without a scope (e.g. chore: bump cargo deps, docs: fix typo in readme). Those have been used in this project historically.
Before landing this, it's worth confirming that the team explicitly wants (scope) to be mandatory for all PR types, including chore: and docs:. If the intent is to enforce scopes only on feature/fix/refactor PRs, the action supports a types: list that can exclude chore and docs.
π΅ Suggestion β consider documenting accepted scopes explicitly
The amannn/action-semantic-pull-request action accepts a scopes input. The project has well-defined subsystems (crates, CI, docs, hardware, web, etc.), and enumerating them serves two purposes: contributors get a clear error message with valid options when they get it wrong, and the set of scopes becomes auditable. If the team prefers open scopes, that's fine β just worth considering at configuration time rather than discovering pain points after merge.
π’ Praise β correct event triggers and minimal permissions
The types: [opened, reopened, edited, synchronize] list is complete. The synchronize trigger is the one that is commonly dropped, causing the check not to re-run after a force-push or new commits β it's here. The permissions: pull-requests: read block is the minimum needed and nothing more. Both are exactly right.
Template note
The validation evidence says "I have no way to try the action locally." That is honest and accurate β you can't dry-run a new GitHub Actions workflow against live PRs without merging or using a fork. The PR body otherwise satisfies the required sections. The security section is appropriately answered.
Summary: Two blockers β SHA pinning and file extension. Both are small and mechanical. Once those are fixed and the scope question is resolved with the team, I expect this to be straightforward to land.
|
@JordanTheJet β milestone alignment needed: this PR does not clearly fit within the scope boundary of any open milestone. Please advise on placement or deferral. |
Summary
master(all contributions)Validation Evidence (required)
Local validation is the signal CI cannot replace. Run the full battery and paste literal output (tails, failures, warnings β not "all passed").
I have no way to try the action locally.
Security & Privacy Impact (required)
Yes/No for each. Answer any
Yeswith a 1β2 sentence explanation.No)No)No)No)Yes, describe the risk and mitigation:Compatibility (required)
Yes)No)NoorYesto either: exact upgrade steps for existing users:Rollback (required for
risk: mediumandrisk: high)Low-risk PRs:
git revert <sha>is the plan unless otherwise noted.Labels live in the GitHub label UI, not in the body. Set
risk:*,size:*, and scope labels via the sidebar. Auto-label corrections: addrisk: manualand the intended label.Commit trailers capture AI-assisted collaboration (
Co-Authored-By: Claude ...) β no separate section needed.Privacy contract (
docs/book/src/contributing/privacy.md) is a merge gate. Never commit real identities, secrets, personal emails, or PII in diff, tests, fixtures, or docs.