Skip to content

fix(ci): lint individual commits on PRs to catch invalid prefixes early#2514

Open
ralphbean wants to merge 1 commit into
mainfrom
fix/commit-lint-pr-commits
Open

fix(ci): lint individual commits on PRs to catch invalid prefixes early#2514
ralphbean wants to merge 1 commit into
mainfrom
fix/commit-lint-pr-commits

Conversation

@ralphbean

Copy link
Copy Markdown
Member

Summary

  • The merge queue uses merge (not squash), so individual commit messages matter
  • commit-lint previously only checked the PR title on pull_request events, deferring per-commit linting to merge_group
  • Invalid prefixes (like style on PR docs(security): add audit log integrity to threat model #2010) only surfaced when the merge queue rejected the PR — too late for contributors to fix easily
  • Now commit-lint checks individual commits on pull_request events too, giving early feedback

Test plan

  • This PR's own commit-lint passes (single commit with valid fix(ci): prefix)
  • Open a PR with a commit using an invalid prefix (e.g., style) — commit-lint should fail on the PR, not wait for the merge queue

The merge queue uses merge (not squash), so individual commit messages
matter. Previously, commit-lint only checked the PR title on
pull_request events and deferred per-commit linting to merge_group.
This meant invalid prefixes (like `style`) only surfaced when the
merge queue rejected the PR — too late for contributors to fix easily.

Now commit-lint checks individual commits on pull_request events too,
giving early feedback before the merge queue.

Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix CI commit-lint to lint individual PR commits early
🐞 Bug fix ⚙️ Configuration changes 🕐 10-20 Minutes

Grey Divider

Description

• Run per-commit gitlint on pull_request events to fail fast on invalid commit prefixes.
• Keep PR-title linting for pull_request, and commit-range linting for push/merge_group.
• Derive the commit range from the triggering event (push/merge_group/pull_request) via a single
 case statement.
Diagram

graph TD
  GH{{"GitHub event"}} --> CL["commit-lint job"] --> PRT["Lint PR title (PR only)"] --> SEL{"Select commit range"} --> LOOP["Iterate commits (no merges)"] --> GL["gitlint commit subjects"]

  subgraph Legend
    direction LR
    _ext{{"External trigger"}} ~~~ _job["CI job/step"] ~~~ _dec{"Decision"}
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Use a dedicated commit-message lint action
  • ➕ Less custom bash/range logic to maintain
  • ➕ Often has built-in PR/commit enumeration support
  • ➖ May not match existing .gitlint rules/ignores exactly
  • ➖ Adds external action dependency and upgrade surface area
2. Enforce squash-merge only (make PR title the single source of truth)
  • ➕ Simplifies linting to PR title only
  • ➕ Avoids per-commit policy enforcement overhead
  • ➖ Conflicts with merge queue behavior (merge commits) described in the PR context
  • ➖ Changes repo workflow expectations for contributors
3. Fetch commit list via GitHub API instead of git ranges
  • ➕ Avoids edge cases where base/head SHAs aren’t present locally
  • ➕ Can precisely target PR commits regardless of checkout state
  • ➖ More code/complexity (API auth, pagination) than current approach
  • ➖ Still needs mapping to local gitlint invocation per commit

Recommendation: The PR’s approach (event-aware SHA range selection and local git rev-list linting) is the best fit given the existing gitlint setup and the need to fail fast on pull_request. The main thing to watch is ensuring the checkout (fetch-depth: 0) continues to provide both PR base/head SHAs; if that ever changes, the GitHub API approach becomes the most robust fallback.

Files changed (1) +12 / -8

Other (1) +12 / -8
lint.ymlLint individual PR commits by selecting a pull_request SHA range +12/-8

Lint individual PR commits by selecting a pull_request SHA range

• Updates the commit-lint job to lint individual commits on 'pull_request' events in addition to 'push' and 'merge_group'. Adds PR base/head SHAs to the environment and replaces the if/else range selection with a 'case' statement to compute the correct commit range per event.

.github/workflows/lint.yml

@github-actions

Copy link
Copy Markdown

Site preview

Preview: https://532634e8-site.fullsend-ai.workers.dev

Commit: d3e9d463c2540aa63e13577f3c8ca01aa4e60541

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 51 rules

Grey Divider


Remediation recommended

1. Silent rev-list failure 🐞 Bug ☼ Reliability
Description
The Lint commits step expands $(git rev-list ...) without fail-fast mode or explicit error
checking, so if RANGE is an invalid revspec (e.g., PUSH_BEFORE is all-zero or a SHA isn’t
available locally), the loop can run zero times and the job still succeeds—skipping commit linting.
This undermines the purpose of adding early per-commit checks on pull_request events.
Code

.github/workflows/lint.yml[R86-94]

+          case "${EVENT_NAME}" in
+            push)          RANGE="${PUSH_BEFORE}..${PUSH_AFTER}" ;;
+            merge_group)   RANGE="${MQ_BASE}..${MQ_HEAD}" ;;
+            pull_request)  RANGE="${PR_BASE}..${PR_HEAD}" ;;
+            *)             echo "Unknown event: ${EVENT_NAME}"; exit 1 ;;
+          esac

          FAILED=false
          for sha in $(git rev-list --no-merges "${RANGE}"); do
Relevance

⭐⭐⭐ High

Team previously fixed silent workflow skips by adding explicit error handling/fail-fast in CI
scripts (e.g., #2398, #191).

PR-#2398
PR-#191
PR-#1565

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The workflow’s shell script does not enable fail-fast behavior and does not check git rev-list’s
exit code, so a failing git rev-list can result in an empty list and a passing job. This is the
same reliability failure mode as prior accepted workflow issues where missing error handling caused
silent skipping.

.github/workflows/lint.yml[76-103]
PR-#2398

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The workflow uses `for sha in $(git rev-list ...)` without `set -e` / `set -o pipefail` or an explicit exit-status check. If `git rev-list` fails due to an invalid range, the command substitution expands to empty, the loop is skipped, and the step can still exit 0.

### Issue Context
This PR makes per-commit linting run on `pull_request` events as well, increasing the importance of not silently skipping commit enumeration.

### Fix Focus Areas
- .github/workflows/lint.yml[85-103]

### Suggested fix approach
- Add `set -euo pipefail` at the start of the `run:` block, OR explicitly validate the range before looping, e.g.:
 - compute `RANGE`
 - run `COMMITS=$(git rev-list --no-merges "${RANGE}")` with `if ! ...; then echo ::error::...; exit 1; fi`
- Optionally handle the known all-zero `before` edge case for pushes by switching to a single-SHA range when `PUSH_BEFORE` is `0000000000000000000000000000000000000000`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 22, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 4:03 PM UTC · Completed 4:16 PM UTC
Commit: d3e9d46 · View workflow run →

@codecov

codecov Bot commented Jun 22, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@fullsend-ai-review

Copy link
Copy Markdown

Review

Findings

High

  • [protected-path] .github/workflows/lint.yml — This PR modifies a file under .github/, which is a protected path requiring human approval. No linked issue provides authorization for this change. While the PR description explains the motivation (catching invalid commit prefixes earlier), protected-path changes require a linked issue for traceability.
    Remediation: File an issue describing the problem (invalid commit prefixes only caught at merge-queue time) and link it to this PR. Human approval is required regardless.

Low

  • [edge-case] .github/workflows/lint.yml:84 — The pull_request range uses github.event.pull_request.base.sha..github.event.pull_request.head.sha. The base SHA is the tip of the target branch at event time, not the merge-base. If main has diverged significantly from the PR's fork point, git rev-list --no-merges could include commits the author did not write, leading to spurious lint failures. Unlikely in practice with typical PR workflows.

  • [scope-alignment] .github/workflows/lint.yml — The PR title uses fix(ci): but per COMMITS.md, fix is for bug fixes visible to users and ci is for CI/CD pipeline changes. This change modifies when validation runs (CI timing), which is better characterized as a ci: or ci(commit-lint): prefix.


Labels: PR modifies CI workflow (.github/workflows/lint.yml)

@fullsend-ai-review fullsend-ai-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the review comment for full details.

MQ_BASE: ${{ github.event.merge_group.base_sha }}
MQ_HEAD: ${{ github.event.merge_group.head_sha }}
PR_BASE: ${{ github.event.pull_request.base.sha }}
PR_HEAD: ${{ github.event.pull_request.head.sha }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] edge-case

The pull_request range uses github.event.pull_request.base.sha..github.event.pull_request.head.sha. The base SHA is the tip of the target branch at event time, not the merge-base. If main has diverged significantly from the PR fork point, git rev-list --no-merges could include commits the author did not write, leading to spurious lint failures. Unlikely in practice with typical PR workflows.

@fullsend-ai-review fullsend-ai-review Bot added the component/ci CI pipelines and checks label Jun 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/ci CI pipelines and checks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant