ci(workflows): add commit message and PR title quality checks

[mrpollo]

Adds CI checks for PR titles and commit messages using conventional commits format (type(scope): description).

Three jobs run on every pull request:

• PR Title (blocking): validates the title matches conventional commits format, suggests a fix
• Commit Messages (blocking + advisory): blocks fixup/WIP/throwaway commits, warns on review-response and formatter-only messages
• PR Description (advisory): warns if the PR body is empty or template-only

Bot comments are concise: issue, suggested fix, link to CONTRIBUTING.md. Comments auto-remove once resolved. Fork PRs get the same checks in CI logs only.

Comment previews: https://gist.github.com/mrpollo/31449b3c9aca89bdbc839d2c5c424464

[mrpollo]

fyi @hamishwillee

[beniaminopozzan]

Thanks @mrpollo !

May I propose to change the commit and pr title format to conventional commit combined with px4 subystem?

Examples:

• feature(ekf2): new feature
• fix(mavlink): command_int frames handling
• ci(ci): add orchestrator
• docs(airframe): add new aiframe

I'm just a fan of conventional commit and I like to be able to spot immediatly if I'm looking at a feature or a fix.

[dakejahl]

I agree with @beniaminopozzan, being able to see whether something is a feature/fix/refactor/etc would be nice. Whatever we decide though we need to enforce consistency. I'd prefer we enforce conventional commits on PR titles only, not individual commits. Here's why:

• Authors keep full commit history during review (no force-push squashing)
• Reviewers can see the full history of changes
• When we hit squash+merge, GitHub uses the PR title as the commit message

To enforce this we can add the semantic-pull-request GitHub Action which validates PR titles against conventional commit format. We should also set the repo default squash commit message to "Pull request title" under Settings --> General --> Pull Requests.

[julianoes]

@dakejahl would we enforce squashing? I was usually against that because sometimes individual commits make sense in case part of a change needs reverting, but I can be convinced otherwise.

[mrpollo]

Thanks for the feedback everyone!

Conventional commits: done, I agree with this approach, it provides much more visibility. @beniaminopozzan I've adopted the type(scope): description format as suggested. The latest push migrates everything to conventional commits. You can see it in action right now: the bot already flagged this PR's own title (suggesting feat(ci): add commit message and PR title quality checks) and both commit messages. The second commit ("addressing pr review comments") is intentionally non-conforming so you can all see what the CI feedback looks like in practice. It'll get squashed before the merge.

One note: the spec uses feat, not feature, so the examples would be feat(ekf2): new feature, fix(mavlink): command_int frames handling, etc.

Squash policy: @dakejahl @julianoes I agree with Julian here. We shouldn't enforce squash-merge across the board. Individual commits are valuable when they're atomic and revertable. This way:

• Contributors keep their commit history during review
• The PR title (which becomes the squash-merge message when reviewers choose that path) is always clean
• If someone has well-structured individual commits, those are preserved as-is
• Reviewers can still request squashing when the history is messy

Open question: Should non-conforming commit messages block CI? Right now, missing conventional commit format on individual commits is just an advisory warning. The only blocking commit checks are for things that should never land on main (fixup!, squash!, WIP, throwaway messages like "oops", tmate leftovers). Should we also make non-conforming commit messages a blocking failure? The tradeoff is a cleaner history on main vs. more friction for external contributors who may not be familiar with the format. Interested to hear what you all think.

Why not just semantic-pull-request? @dakejahl, that action only validates PR titles. Since we want to preserve individual commits when they're clean, we need commit-level advisory checks as well (catching fixup!, WIP, and throwaway messages like "oops"). The custom scripts also provide PX4-specific scope suggestions and auto-commenting with fix instructions, which the off-the-shelf action doesn't.

What changed in this push:

• Migrated validation to type(scope): description format with all 11 conventional commit types
• Added shared conventional_commits.py module (types, regex, parser, scope/type suggestion helpers)
• Updated CONTRIBUTING.md, docs/contribute/code.md, and git_examples.md to document the new format
• Added issues: write to workflow permissions (flagged by the security bot)
• Consolidated the double script execution in the workflow (each job now runs the checker once instead of twice)

[github-actions]

No broken links found in changed files.

[dakejahl]

The choice between squash-merge and multi-commit-merge is on a case by case basis. 99% of PRs are squash-merge. The PR title should be formatted correctly regardless of whether the reviewer chooses to squash-merge or multi-commit-merge. I think we should enforce it to get people in the habit of doing it. Maybe the CI failure can comment on the PR and suggest a title, this way a reviewer can easily copy/paste the suggestion and update the PR description if the author is ignorant.

[mrpollo]

@dakejahl Cool I think thats aligned with my latest change, which as you can see above added a comment, which explains the error and also proposes a fixed title, plus it failed the checks so we can't merge.

[dakejahl]

Commit Messages (advisory)

This is not blocking, but we noticed some commit messages that could be improved.

Commit Message Suggestion
c13f937af1 CI: add commit message and PR title quality checks Missing conventional commit format (e.g. "feat(ekf2): add something")
f198d02131 addressing pr review comments Missing conventional commit format (e.g. "feat(ekf2): add something")
Why this matters: every commit on main is permanent project history. Clean, conventional commit messages (type(scope): description) make git log, git blame, and release notes much more useful.

Consider squashing (combining) review-response commits ("address review", "apply suggestions") and formatting commits ("make format") into their parent commit before merge:

git rebase -i HEAD~2
# change 'pick' to 'squash' (or 's') for the commits to combine
git push --force-with-lease  # safe for PR branches

PX4 conventional commit format
PX4 uses conventional commits for all commit messages and PR titles. This keeps git log and git blame readable and makes it easy to generate changelogs.

Format: type(scope): description

Types:

Type Description
feat A new feature
fix A bug fix
docs Documentation only changes
style Formatting, whitespace, no code change
refactor Code change that neither fixes a bug nor adds a feature
perf Performance improvement
test Adding or correcting tests
build Build system or external dependencies
ci CI configuration files and scripts
chore Other changes that don't modify src or test files
revert Reverts a previous commit
The scope identifies the part of PX4 affected (e.g. ekf2, mavlink, boards/px4_fmu-v6x). Look at the directory path of your changed files to find the right scope.

Append ! before the colon to mark a breaking change:

feat(ekf2)!: remove deprecated height fusion API

Good commit messages:

feat(ekf2): add height fusion timeout
fix(mavlink): correct BATTERY_STATUS_V2 parsing
refactor(navigator): simplify RTL altitude logic
ci(workflows): migrate to reusable workflows

Commits to avoid (squash these before merging):

fix                          # too vague
apply suggestions from code review  # squash into parent
do make format               # squash into parent
WIP: trying something        # not ready for main

See the full commit message convention in the contributing guide.

This comment will be automatically removed once the issues are resolved.

The drop-down with explanation is fine, but the above message is overly verbose. LLMs in-general annoy me with their verbosity. We don't need to explain the justification for the formatting, or at least move that to the drop down. Also we shouldn't suggest people to change commit messages, I want to avoid force-pushing since it erases history forcing a full re-review. Also for noobs it can cause them to accidentally blow away their changes. Since the 99% case is squash-merge and the commit message comes from the PR title, only suggesting/enforcing PR titles seems like the best solution.

[mrpollo]

@dakejahl this is ready i made the changes we discussed, here's the preview for the new comments w/o the AI slop https://gist.github.com/mrpollo/31449b3c9aca89bdbc839d2c5c424464

[dakejahl]

Review-response commit: consider squashing before merge;

I don't think force pushing after review is a good idea because changes can slip in undetected with a force push from the author. Either unintentionally or maliciously regressions can be introduced, especially when we ask for a rebase against main.

Other than that I think it's okay to enforce commits to follow the convention. Also by using the squash-merge strategy you can merge main into your branch without polluting the state of review. It all goes in as one commit at the end of the day with squash-merge anyway (unless the author has a good reason for separate commits, which we handle on a case by case basis)