From f3fd8b880554ec01a261145aceb65aaafd4ea27c Mon Sep 17 00:00:00 2001 From: Ian Leggett Date: Wed, 23 Sep 2026 14:49:12 +0000 Subject: [PATCH] fix: skip pre-commit hooks when merging a branch to a PR Signed-off-by: DBT pre-commit check --- .github/workflows/org.common-ci.yml | 59 +++++++++++++++++++++++++---- README.md | 19 ++++++++++ 2 files changed, 70 insertions(+), 8 deletions(-) diff --git a/.github/workflows/org.common-ci.yml b/.github/workflows/org.common-ci.yml index 4dfde1e7..50003c11 100644 --- a/.github/workflows/org.common-ci.yml +++ b/.github/workflows/org.common-ci.yml @@ -15,7 +15,14 @@ jobs: if: ${{ github.actor != 'dependabot[bot]' }} env: SIGNED_OFF_MESSAGE: "Signed-off-by: DBT pre-commit check" - FAILURE_MESSAGE: "Your PR has commits that are missing the Signed-off-by trailer. This is likely due to the pre-commit hook not being configured on your local machine. The usual fix for this issue is to run `pre-commit install --install-hooks --overwrite -t commit-msg -t pre-commit`, however for more detailed help in setting up the pre-commit hooks, follow the instructions at https://github.com/uktrade/github-standards/blob/main/README.md#usage" + # Commits made in the GitHub web UI cannot run the pre-commit hook. Commits by this committer whose + # message starts with one of these lines are allowed to skip the check. $BASE_REF is the PR's target branch + WEB_UI_COMMITTER_EMAIL: "noreply@github.com" + WEB_UI_COMMIT_MESSAGES: | + Apply suggestion from + Apply suggestions from + Merge branch '$BASE_REF' into + FAILURE_MESSAGE: "Your PR has commits that are missing the Signed-off-by trailer, which is added by the pre-commit hooks. To install them, run `pre-commit install --install-hooks --overwrite -t commit-msg -t pre-commit`, or follow the instructions at https://github.com/uktrade/github-standards/blob/main/README.md#usage. If the commit was made in the GitHub web UI, installing the hooks will not help. Only applying suggested changes and updating the branch are allowed there, so make the change locally instead" runs-on: ubuntu-latest permissions: @@ -26,22 +33,58 @@ jobs: uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd with: ref: ${{ github.event.pull_request.head.sha }} + fetch-depth: 0 - name: Check if pre-commit hook ran before push id: pre-commit-check + env: + GH_TOKEN: ${{ github.token }} + BASE_REF: ${{ github.event.pull_request.base.ref }} + BASE_SHA: ${{ github.event.pull_request.base.sha }} run: | - # If this commit belongs to a github PR, it will not have the git trailer that is set by + # Was this commit made in the GitHub web UI by one of the actions we allow to skip the check? + is_allowed_web_ui_commit() { + local email="$1" message="$2" allowed + [ "$email" == "$WEB_UI_COMMITTER_EMAIL" ] || return 1 + while IFS= read -r allowed; do + allowed="${allowed//\$BASE_REF/$BASE_REF}" + if [ -n "$allowed" ] && [[ "$message" == "$allowed"* ]]; then + return 0 + fi + done <<< "$WEB_UI_COMMIT_MESSAGES" + return 1 + } + + # Find the latest commit in the PR that was not made by one of those allowed web UI actions, + # following the PR branch side of any merge commit. That will either be a local commit, which + # should have the trailer, or another web UI commit, such as a file edited in the browser, which won't + sha="" + while read -r commit_sha email message; do + if is_allowed_web_ui_commit "$email" "$message"; then + echo "Skipping commit $commit_sha, it was made in the GitHub web UI: $message" + continue + fi + sha="$commit_sha" + break + done < <(git log --first-parent --format='%H %ce %s' "$BASE_SHA..HEAD") + + if [ -z "$sha" ]; then + echo "Every commit in this PR was made in the GitHub web UI by an action that cannot run the pre-commit hook" + exit 0 + fi + + # If this commit belongs to a github PR, it will not have the git trailer that is set by # the pre-commit hook. If the response is NOT an empty array, that means the commit was from # a PR and we can skip the git trailer checks - - is_pr_commit=$(gh pr list --search ${{ github.event.pull_request.head.sha }} --state merged --json id | jq '. != []') + is_pr_commit=$(gh pr list --search "$sha" --state merged --json id | jq '. != []') if [ "$is_pr_commit" == true ]; then - echo "This commit came from a PR, it will not have the trailer from a pre-commit hook" + echo "Commit $sha came from a merged PR, it will not have the trailer from a pre-commit hook" exit 0 fi - echo "We need to check this for the trailers" - git log ${{ github.event.pull_request.head.sha }} --format=%B -1 | git interpret-trailers --parse | grep '${{ env.SIGNED_OFF_MESSAGE }}' + + echo "Checking commit $sha for the trailer" + git log "$sha" --format=%B -1 | git interpret-trailers --parse | grep '${{ env.SIGNED_OFF_MESSAGE }}' - name: Find failure comment uses: peter-evans/find-comment@b30e6a3c0ed37e7c023ccd3f1db5c6c0b0c23aad @@ -114,4 +157,4 @@ jobs: ${{ env.DOCKER_IMAGE }}:$tag \ run_scan \ --github-action \ - /src + /src \ No newline at end of file diff --git a/README.md b/README.md index 4d888c0c..1e784fa5 100644 --- a/README.md +++ b/README.md @@ -26,6 +26,7 @@ - [Upgrading bandit](#upgrading-bandit) - [GitHub actions](#github-actions) - [Testing changes](#testing-changes) + - [Signed-off-by trailer check](#signed-off-by-trailer-check) - [FAQ](#faq) - [My PR is failing due to a github action checking a Signed-off-by trailer](#my-pr-is-failing-due-to-a-github-action-checking-a-signed-off-by-trailer) - [I'm receiving errors updating the rev version](#im-receiving-errors-updating-the-rev-version) @@ -169,6 +170,24 @@ There is a `bandit-version` `env` variable in this job, that is used to install This repository contains GitHub actions that are triggered by a set of GitHub Rulesets defined at the organisation level. Any repository in the uktrade organisation can opt in to using these GitHub actions by adding GitHub Custom properties to the repository. +## Signed-off-by trailer check + +The `pre-commit-check` job in `org.common-ci.yml` verifies that commits were made after installing the pre-commit hooks from this repo, since the hooks are what runs the security and personal data scans locally before a commit is allowed. It does this by checking for the `Signed-off-by: DBT pre-commit check` trailer that the commit-msg hook adds to a commit message once the scans pass - a commit without this trailer means the hooks were either not installed, or were bypassed with `--no-verify`. + +Not every commit can realistically carry this trailer though. Commits made directly in the GitHub web UI (for example applying a suggested change, or a merge commit created by clicking "Update branch") never run the local hook, so the job needs to tell those apart from a commit that was made locally and skipped the hooks. It does this by walking the PR's commits (following first-parent only, so merged-in history from `main`/`master`/`dev` is ignored) from newest to oldest, skipping over commits that match a known, safe web UI pattern, until it finds the first commit that must be checked. That commit passes if it either came from a PR that was already merged (so it wouldn't have been run through this check locally), or if it contains the trailer. The table below covers every outcome: + +| Scenario | Condition | Outcome | Why | +|---|---|---|---| +| Every commit in the PR is an allowed web UI commit | All commits have committer email `noreply@github.com` **and** a message matching an allowed prefix (`Apply suggestion from`, `Apply suggestions from`, `Merge branch '$BASE_REF' into`) | ✅ Pass | Nothing to check, the job exits early | +| Latest non-web-UI commit came from an already-merged PR | `gh pr list --search "$sha" --state merged` returns a non-empty result | ✅ Pass | The commit predates/bypassed the trailer check via a merge, so it's exempted | +| Latest non-web-UI commit has the trailer | Commit message contains `Signed-off-by: DBT pre-commit check` | ✅ Pass | The pre-commit hook was installed and ran correctly | +| Latest non-web-UI commit is missing the trailer | Pre-commit hook wasn't installed/run, no trailer in message | ❌ Fail | This is the case the FAQ bullets above address | +| Web UI commit from `noreply@github.com` with a message that doesn't match any allowed prefix (e.g. a manual file edit made in the browser) | Email matches, but message text doesn't start with an allowed prefix | ❌ Fail (usually) | Not treated as an allowed web UI commit, so it's the commit that gets checked - browser edits don't carry the trailer | +| Non-web-UI committer email, any message | Committer email isn't `noreply@github.com` | Depends on trailer | This is always the commit that gets checked, since it can never match the allow-list | +| Merge commit pulled in via a non-first-parent branch | Commit reachable only through the second parent of a merge | *(ignored)* | `--first-parent` means these commits are never examined | +| PR opened by `dependabot[bot]` | `github.actor == 'dependabot[bot]'` | Skipped entirely | The job doesn't run at all for dependabot PRs | + + ## Terraform Workflow The reusable Terraform workflow defined in this repository checks Terraform code in your repository against a number of standard tools: `terraform fmt`, `terraform validate` and `tflint`. If any of these checks do not exit successfully, the job will fail and you will need to make changes to your code to get it through the CI checks. Because a lot of the Terraform modules we use in our code are hosted in private GitHub repositories, we have had to create a GitHub App to allow them to be pulled into the GitHub Action at runtime. Therefore, there are some pre-requisites you must satisfy before this reusable workflow will work on your repository: