FXC-3354-Lint-commit-messages #2862

mahlau-flex · 2025-10-02T06:57:59Z

Added commitlint to pre-commit pipeline and github wokflow

Greptile Overview

Updated On: 2025-10-02 06:59:20 UTC

Summary

This PR implements commit message linting across the Tidy3D project by adding commitlint configuration and integrating it into both the pre-commit pipeline and GitHub Actions workflow. The changes introduce standardized commit message formatting based on conventional commit standards.

The implementation adds three key components:

Configuration file (.commitlint.yml) that defines strict rules for commit messages including mandatory commit types (feat, fix, docs, style, refactor, perf, test, build, ci, chore, revert), length constraints (12-72 characters for headers), and lowercase scoping requirements
Pre-commit integration that validates commit messages locally during the commit process using the alessandrojcm/commitlint-pre-commit-hook
GitHub Actions job that validates all commit messages in PRs or single commits in pushes using a Go-based commitlint tool

The enforcement follows a gradual rollout strategy - currently configured in informational mode to show warnings but not block merges, with plans to make it mandatory in the future. This approach allows developers to adapt to the new requirements without disrupting existing workflows. The GitHub Actions implementation intelligently handles different event types, checking all commits in a PR range versus just the head commit for pushes.

This change fits into the broader Tidy3D codebase quality infrastructure, complementing existing pre-commit hooks and automated testing pipelines. It establishes a foundation for improved commit history readability, automated changelog generation, and better project maintainability.

PR Description Notes:

Minor typo: "wokflow" should be "workflow"

Important Files Changed

Changed Files

Filename	Score	Overview
`.commitlint.yml`	5/5	New configuration file defining conventional commit rules with appropriate type enforcement and length constraints
`.pre-commit-config.yaml`	4/5	Added commitlint hook integration to validate commit messages during local development
`.github/workflows/tidy3d-python-client-tests.yml`	4/5	Added GitHub Actions job to validate commit messages in CI pipeline with proper PR/push handling

Confidence score: 4/5

This PR is safe to merge with minimal risk as it introduces commit message validation without breaking existing functionality
Score reflects well-structured implementation with proper gradual rollout strategy and comprehensive integration across development workflows
Pay close attention to the GitHub Actions workflow implementation to ensure the commit message extraction logic works correctly for all event types

Sequence Diagram

sequenceDiagram
    participant Developer
    participant Git
    participant PreCommit as "Pre-commit Hooks"
    participant CommitLint as "CommitLint"
    participant GitHub as "GitHub Actions"
    participant RuffLinter as "Ruff Linter"
    participant TestRunner as "Test Runner"

    Developer->>Git: "git commit -m 'feat: add new feature'"
    Git->>PreCommit: "Trigger pre-commit hooks"
    PreCommit->>RuffLinter: "Run ruff-check and ruff-format"
    RuffLinter-->>PreCommit: "Code formatting results"
    PreCommit->>CommitLint: "Validate commit message format"
    CommitLint-->>PreCommit: "Commit message validation result"
    PreCommit-->>Git: "Pre-commit validation complete"
    Git-->>Developer: "Commit accepted/rejected"

    Developer->>GitHub: "Push commits to PR branch"
    GitHub->>GitHub: "Trigger tidy3d-python-client-tests workflow"
    GitHub->>GitHub: "Determine test scope based on PR status"
    
    par Linting Jobs
        GitHub->>RuffLinter: "Run ruff format --check --diff"
        RuffLinter-->>GitHub: "Format check results"
        GitHub->>RuffLinter: "Run ruff check tidy3d"
        RuffLinter-->>GitHub: "Lint check results"
    and Commit Message Linting
        GitHub->>CommitLint: "Check all PR commit messages"
        CommitLint-->>GitHub: "Commit message validation results"
    and Schema Verification
        GitHub->>GitHub: "Verify schema changes"
        GitHub-->>GitHub: "Schema validation results"
    end

    GitHub->>TestRunner: "Run local tests (Python 3.10, 3.13)"
    TestRunner-->>GitHub: "Local test results"

    alt PR is approved
        GitHub->>TestRunner: "Run remote tests (multiple platforms)"
        TestRunner-->>GitHub: "Remote test results"
    end

    GitHub->>GitHub: "Check pr-requirements-pass"
    GitHub-->>Developer: "PR status update"

greptile-apps

Additional Comments (1)

.github/workflows/tidy3d-python-client-tests.yml, line 524 (link)

logic: Missing 'lint-commit-messages' dependency in pr-requirements-pass job needs array

_{3 files reviewed, 4 comments}

_{Edit Code Review Agent Settings | Greptile}

.commitlint.yml

.github/workflows/tidy3d-python-client-tests.yml

frederikschubertflex · 2025-10-02T07:09:11Z

I like this! 👍 However, if we always to squash and merge and are using the merge queue, couldn't we enforce the commit message linting only for the commit that lands in main (develop)?
Or is this already how this is supposed to work?

yaugenst-flex · 2025-10-02T07:11:24Z

I like this! 👍 However, if we always to squash and merge and are using the merge queue, couldn't we enforce the commit message linting only for the commit that lands in main (develop)? Or is this already how this is supposed to work?

Great point. This should probably only be enforced once we queue for merge? And then for all commits (in case there is more than one, which does happen from time to time). Nice side-effect is that this would probably also catch cases where we forget to squash.

mahlau-flex · 2025-10-02T07:25:58Z

What exactly do you mean that I should change then? Is there an option to run the commitlint-Pipeline only if the PR is merged? I feel like it would be better to immediately have the feedback as soon as the PR is opened rather than on merge.

mahlau-flex · 2025-10-02T07:27:10Z

Currently, the github pipeline is not working at all, I will see how to fix this

github-actions · 2025-10-02T07:45:23Z

Diff Coverage

Diff: origin/develop...HEAD, staged and unstaged changes

No lines with coverage information in this diff.

mahlau-flex · 2025-10-02T07:59:29Z

I added a check, which fails the PR if there is more than a single commit present. Is there any case where we might want to have more than a single commit in the PR? If this check is too strict, we can also remove it again. @yaugenst-flex could you give feedback here?

yaugenst-flex · 2025-10-02T08:09:43Z

I added a check, which fails the PR if there is more than a single commit present. Is there any case where we might want to have more than a single commit in the PR? If this check is too strict, we can also remove it again. @yaugenst-flex could you give feedback here?

So I think we should allow for more than one commit per PR, because sometimes it does make sense to split up to changes within one PR into two logical commits. That should not be the norm, but it does happen. Sometimes however we just forget to squash, and in those cases typically the individual commits will also not follow commitlint rules (might be commits added after review comments for example). And if we enforce commitlint on all commits, then we would catch these cases automatically.

I also think we might not want to enforce commitlint rules on PR open but instead only when queuing up for merge? Since that‘s really the only place where it matters.

daquinteroflex · 2025-10-02T08:15:47Z

So yes, by default the merge queue will rebase the PR commits, and I guess we want to check every commit in the rebase, that said it can be quite annoying to have this check other than in the merge_group event, so it'd be good for this to be triggered on those only.

.github/workflows/tidy3d-python-client-tests.yml

mahlau-flex · 2025-10-02T08:36:37Z

So yes, by default the merge queue will rebase the PR commits, and I guess we want to check every commit in the rebase, that said it can be quite annoying to have this check other than in the merge_group event, so it'd be good for this to be triggered on those only.

I changed the github pipeline to only run on merge-group events

mahlau-flex · 2025-10-02T08:37:32Z

Does it make sense to change the local pre-commit config now as well? I am not sure how this should be configured now

.github/workflows/tidy3d-python-client-tests.yml

docs/development/introduction/index.rst

daquinteroflex · 2025-10-02T08:51:23Z

Looks broadly good! Not sure if any further comments before moving forward?

daquinteroflex · 2025-10-02T08:56:50Z

Does it make sense to change the local pre-commit config now as well? I am not sure how this should be configured now

default_install_hook_types:
  - pre-commit
  - commit-msg

https://pre-commit.com/#top_level-default_install_hook_types

Maybe means pre-commit install will work automatically for people like before`

.github/workflows/tidy3d-python-client-tests.yml

mahlau-flex · 2025-10-02T10:24:18Z

Does it make sense to change the local pre-commit config now as well? I am not sure how this should be configured now
default_install_hook_types:
  - pre-commit
  - commit-msg
https://pre-commit.com/#top_level-default_install_hook_types

Maybe means pre-commit install will work automatically for people like before`

Good suggestions, this works locally for me

mahlau-flex · 2025-10-02T10:54:01Z

I have changed the pre-commit pipeline, such that the user will get a warning if the commit message is bad, but the pre-commit pipeline does not fail. With this, we can make bad commit messages (temporarily) and squash them together later with a good commit message.

.github/workflows/tidy3d-python-client-tests.yml

yaugenst-flex

So if I read this correctly, then pr-requirements-pass now depends on lint-commit-messages, but that depends on if: github_event_name == 'merge_group'. This will be skipped on every PR event, which means pr-requirements-pass will never run (correct my understanding if wrong, not a github actions expert...). I think the fix would be to drop the job-level if so that the required dependency is always satisfied and then make the actual commitlint work conditional inside the job (if: github.event_name == 'merge_group').

.github/workflows/tidy3d-python-client-tests.yml

daquinteroflex · 2025-10-02T12:19:07Z

@yaugenst-flex isn't it because of always() in

tidy3d/.github/workflows/tidy3d-python-client-tests.yml

Lines 513 to 517 in ea80d7c

    
                 always() && 
        
                 ((github.event_name == 'pull_request') || (github.event_name == 'pull_request_review') || (github.event_name == 'merge_group' ))  &&  
        
                 (( needs.determine-test-scope.outputs.pr_approval_state == 'true' ) &&  
        
                 ( needs.determine-test-scope.outputs.local_tests == 'true' ) ||  
        
                 ( needs.determine-test-scope.outputs.remote_tests == 'true' ))

this overrides the needs.lint-commit-messages skip event for pr-requirements-pass? It was intended to override the needs logic intentionally

I've used it in some other actions and works well, and should cover this situation I think

yaugenst-flex · 2025-10-02T12:31:57Z

@yaugenst-flex isn't it because of always() in

tidy3d/.github/workflows/tidy3d-python-client-tests.yml

Lines 513 to 517 in ea80d7c

always() &&

((github.event_name == 'pull_request') || (github.event_name == 'pull_request_review') || (github.event_name == 'merge_group' )) &&

(( needs.determine-test-scope.outputs.pr_approval_state == 'true' ) &&

( needs.determine-test-scope.outputs.local_tests == 'true' ) ||

( needs.determine-test-scope.outputs.remote_tests == 'true' ))

this overrides the needs.lint-commit-messages skip event for pr-requirements-pass? It was intended to override the needs logic intentionally

I've used it in some other actions and works well, and should cover this situation I think

I'm not sure, but we can try it. My understanding is that always() works well in cases where the job actually runs but fails. But here, lint-commit-messages would be a dependency that never runs (https://docs.github.com/en/actions/using-jobs/using-conditions-to-control-job-execution#if-failure).

yaugenst-flex · 2025-10-02T12:33:16Z

Ah but I guess we could of course let the commitlint run and fail, in which case your guard would catch it. Seems a bit wasteful though...

daquinteroflex · 2025-10-02T13:02:15Z

So if we add the if github.event == merge_group here

tidy3d/.github/workflows/tidy3d-python-client-tests.yml

Lines 163 to 166 in ea80d7c

    
           lint-commit-messages: 
        
             needs: determine-test-scope 
        
             runs-on: ubuntu-latest 
        
             name: veryify-commit-linting

then it would just skip that job, and and also then be skipped by a similar if statement in the pr-requirements-pass and that'd be fine?

Ideally though some flag like needs.determine-workflow-scope.merge_group can be helpful for managing that state though but as long as it's the same logic that's ok too

yaugenst-flex · 2025-10-02T13:47:25Z

No in what i’m proposing, the job would pass (by skipping its steps), and only truly run when going in merge queue.

daquinteroflex · 2025-10-02T13:56:41Z

So I think both approaches get ultimately the same PR-facing behaviour - happy to try them out! My personal preference is skipping it at the job level like we do other jobs, and simply skipping the check in a merge_group. Ultimately though, I think we're both intending to skip it until the check is meant to run from what I understand you're proposing, it's just where we skip

yaugenst-flex · 2025-10-02T14:25:45Z

Yes let’s just try. What’s the worst that could happen 😄

mahlau-flex · 2025-10-06T06:20:38Z

@yaugenst-flex @frederikschubertflex Do you want to see any other changes or this it PR ready to merge? If so please let me know.

yaugenst-flex · 2025-10-07T05:03:45Z

So I think both approaches get ultimately the same PR-facing behaviour - happy to try them out! My personal preference is skipping it at the job level like we do other jobs, and simply skipping the check in a merge_group. Ultimately though, I think we're both intending to skip it until the check is meant to run from what I understand you're proposing, it's just where we skip

So this is what I mean #2854

mahlau-flex requested a review from yaugenst-flex October 2, 2025 06:58

greptile-apps bot reviewed Oct 2, 2025

View reviewed changes

.commitlint.yml Outdated Show resolved Hide resolved

.github/workflows/tidy3d-python-client-tests.yml Show resolved Hide resolved

.github/workflows/tidy3d-python-client-tests.yml Outdated Show resolved Hide resolved

yaugenst-flex requested a review from daquinteroflex October 2, 2025 06:59

yaugenst-flex assigned mahlau-flex Oct 2, 2025

yaugenst-flex requested a review from frederikschubertflex October 2, 2025 07:00

mahlau-flex force-pushed the FXC-3354-Lint-commit-messages branch from 1b135b2 to b817290 Compare October 2, 2025 07:08

mahlau-flex force-pushed the FXC-3354-Lint-commit-messages branch 2 times, most recently from 75d3f0a to e2c5451 Compare October 2, 2025 07:56

daquinteroflex reviewed Oct 2, 2025

View reviewed changes

.github/workflows/tidy3d-python-client-tests.yml Show resolved Hide resolved

mahlau-flex force-pushed the FXC-3354-Lint-commit-messages branch from e2c5451 to 0c9acd8 Compare October 2, 2025 08:35

daquinteroflex reviewed Oct 2, 2025

View reviewed changes

.github/workflows/tidy3d-python-client-tests.yml Outdated Show resolved Hide resolved

daquinteroflex reviewed Oct 2, 2025

View reviewed changes

docs/development/introduction/index.rst Outdated Show resolved Hide resolved

daquinteroflex reviewed Oct 2, 2025

View reviewed changes

.github/workflows/tidy3d-python-client-tests.yml Outdated Show resolved Hide resolved

mahlau-flex force-pushed the FXC-3354-Lint-commit-messages branch from 0c9acd8 to 6a47a17 Compare October 2, 2025 10:52

daquinteroflex reviewed Oct 2, 2025

View reviewed changes

.github/workflows/tidy3d-python-client-tests.yml Outdated Show resolved Hide resolved

mahlau-flex force-pushed the FXC-3354-Lint-commit-messages branch from 6a47a17 to ded7534 Compare October 2, 2025 11:46

yaugenst-flex requested changes Oct 2, 2025

View reviewed changes

.github/workflows/tidy3d-python-client-tests.yml Outdated Show resolved Hide resolved

mahlau-flex force-pushed the FXC-3354-Lint-commit-messages branch from ded7534 to ea80d7c Compare October 2, 2025 12:03

frederikschubertflex approved these changes Oct 6, 2025

View reviewed changes

yaugenst-flex approved these changes Oct 6, 2025

View reviewed changes

feat: added commitlint pipeline

3108ae0

yaugenst-flex force-pushed the FXC-3354-Lint-commit-messages branch from ea80d7c to 3108ae0 Compare October 6, 2025 10:21

yaugenst-flex enabled auto-merge October 6, 2025 10:21

yaugenst-flex added this pull request to the merge queue Oct 6, 2025

Merged via the queue into develop with commit fbeeb61 Oct 6, 2025
22 checks passed

yaugenst-flex deleted the FXC-3354-Lint-commit-messages branch October 6, 2025 11:33

mahlau-flex mentioned this pull request Oct 7, 2025

feat: fixed commitlint check in github actions #2868

Merged

FXC-3354-Lint-commit-messages #2862

FXC-3354-Lint-commit-messages #2862

Uh oh!

Conversation

mahlau-flex commented Oct 2, 2025 • edited by greptile-apps bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Overview

Summary

Important Files Changed

Confidence score: 4/5

Sequence Diagram

Uh oh!

greptile-apps bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Additional Comments (1)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

frederikschubertflex commented Oct 2, 2025

Uh oh!

yaugenst-flex commented Oct 2, 2025

Uh oh!

mahlau-flex commented Oct 2, 2025

Uh oh!

mahlau-flex commented Oct 2, 2025

Uh oh!

github-actions bot commented Oct 2, 2025

Diff Coverage

Diff: origin/develop...HEAD, staged and unstaged changes

Uh oh!

mahlau-flex commented Oct 2, 2025

Uh oh!

yaugenst-flex commented Oct 2, 2025

Uh oh!

daquinteroflex commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mahlau-flex commented Oct 2, 2025

Uh oh!

mahlau-flex commented Oct 2, 2025

Uh oh!

Uh oh!

Uh oh!

daquinteroflex commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daquinteroflex commented Oct 2, 2025

Uh oh!

Uh oh!

mahlau-flex commented Oct 2, 2025

Uh oh!

mahlau-flex commented Oct 2, 2025

Uh oh!

Uh oh!

yaugenst-flex left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

daquinteroflex commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yaugenst-flex commented Oct 2, 2025

Uh oh!

yaugenst-flex commented Oct 2, 2025

Uh oh!

daquinteroflex commented Oct 2, 2025

Uh oh!

yaugenst-flex commented Oct 2, 2025

Uh oh!

daquinteroflex commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yaugenst-flex commented Oct 2, 2025

Uh oh!

mahlau-flex commented Oct 6, 2025

Uh oh!

mahlau-flex commented Oct 2, 2025 •

edited by greptile-apps bot

Loading

greptile-apps bot left a comment •

edited

Loading

daquinteroflex commented Oct 2, 2025 •

edited

Loading

daquinteroflex commented Oct 2, 2025 •

edited

Loading

yaugenst-flex left a comment •

edited

Loading

daquinteroflex commented Oct 2, 2025 •

edited

Loading

daquinteroflex commented Oct 2, 2025 •

edited

Loading