yonlu/yellowjacket
1
1 Fork 0
Code Issues 63 Pull Requests 1 Actions Packages Projects Releases 13 Wiki Activity

commit-check on a PR lints only the head commit, so broken commits merge green #254

New Issue
Closed
opened 2026-09-11 21:15:29 +00:00 by yonlu · 1 comment
yonlu commented 2026-09-11 21:15:29 +00:00
Owner
Copy link
Copy source

commit-check on a PR lints only the head commit, so a commit with an over-length subject merges under a green PR and fails main's post-merge run. Measured twice.

The mechanism: Gitea leaves github.event.before empty on pull_request events, so the Commit messages step falls back to bare make commit-check, which checks git log -1 only. A push to main carries a real before and lints every non-merge commit in the range — the first examination any of the PR's older commits ever gets.

PR #245: its final CI (run 19567) linted the tip 68e7edb8 ("feat(database): listening-events log with skip counters", 61 chars) and was green; a3b5b437 ("fix(database): preserve playlist phantoms across a stale audio_files retire", 75 chars) was the second commit on the branch and was never linted. The post-merge push run (19571) linted 5fae61cd..6aeac42a, caught it, and left main red with e2e skipped (needs: check). The same log's css-check: node: not found is cascade noise: css-check runs under if: !cancelled(), but every toolchain step between it and the failure was skipped, so node was never installed.

The repeat is what makes this a gap rather than one long subject: PR #253's head was 80 chars and its PR run (19577) failed the same check — red there, so the loop would have refused to merge it, but the a3b5b437 case shows the first commit of a two-commit push can slip through entirely. The local guard is not a backstop either: the commit-msg hook prints Can't find lefthook in PATH and proceeds whenever lefthook is not installed, which is how both subjects got committed.

Proposed fix, in the Commit messages step of .gitea/workflows/ci.yml: pick the base sha on PR events, which makes a green PR a promise about every commit its merge would bring:

env:
  BEFORE: ${{ github.event_name == 'pull_request' && github.event.pull_request.base.sha || github.event.before }}

(Linting base.sha..head lints the PR's own commits: base advances on main, so the old base commits the branch shares are still reachable from before and excluded. The same step already handles the push event unchanged.)

`commit-check` on a PR lints only the head commit, so a commit with an over-length subject merges under a green PR and fails main's post-merge run. Measured twice. The mechanism: Gitea leaves `github.event.before` empty on `pull_request` events, so the Commit messages step falls back to bare `make commit-check`, which checks `git log -1` only. A push to main carries a real `before` and lints every non-merge commit in the range — the first examination any of the PR's older commits ever gets. PR #245: its final CI (run 19567) linted the tip `68e7edb8` ("feat(database): listening-events log with skip counters", 61 chars) and was green; `a3b5b437` ("fix(database): preserve playlist phantoms across a stale audio_files retire", 75 chars) was the second commit on the branch and was never linted. The post-merge push run (19571) linted `5fae61cd..6aeac42a`, caught it, and left main red with e2e skipped (`needs: check`). The same log's `css-check: node: not found` is cascade noise: `css-check` runs under `if: !cancelled()`, but every toolchain step between it and the failure was skipped, so node was never installed. The repeat is what makes this a gap rather than one long subject: PR #253's head was 80 chars and its PR run (19577) failed the same check — red there, so the loop would have refused to merge it, but the a3b5b437 case shows the first commit of a two-commit push can slip through entirely. The local guard is not a backstop either: the commit-msg hook prints `Can't find lefthook in PATH` and proceeds whenever lefthook is not installed, which is how both subjects got committed. Proposed fix, in the Commit messages step of `.gitea/workflows/ci.yml`: pick the base sha on PR events, which makes a green PR a promise about every commit its merge would bring: ```yaml env: BEFORE: ${{ github.event_name == 'pull_request' && github.event.pull_request.base.sha || github.event.before }} ``` (Linting `base.sha..head` lints the PR's own commits: base advances on main, so the old base commits the branch shares are still reachable from `before` and excluded. The same step already handles the push event unchanged.)
yonlu added the Area/Packaging
Priority
Medium
3
Kind/Bug labels 2026-09-11 21:15:29 +00:00
yonlu self-assigned this 2026-09-23 11:25:59 +00:00
yonlu added the
Status
In Progress
 label 2026-09-23 11:26:00 +00:00
yonlu commented 2026-09-23 11:26:00 +00:00
Author
Owner
Copy link
Copy source

Claiming. The proposed patch is the fix: BEFORE becomes github.event.pull_request.base.sha on pull_request and stays github.event.before otherwise, so a green PR is a promise about every commit its merge would bring rather than about the tip alone. Nothing below the assignment changes — the empty/all-zeros/unknown-commit fallback and the RANGE argument stay as they are.

Verification is honest about its limits: the range logic is exercised locally with a real A..B containing a known-bad subject, and the YAML is validated; the end-to-end proof is the next PR with an over-length commit that is not its tip.

Branch batch/168-228-254.

Claiming. The proposed patch is the fix: `BEFORE` becomes `github.event.pull_request.base.sha` on `pull_request` and stays `github.event.before` otherwise, so a green PR is a promise about every commit its merge would bring rather than about the tip alone. Nothing below the assignment changes — the empty/all-zeros/unknown-commit fallback and the `RANGE` argument stay as they are. Verification is honest about its limits: the range logic is exercised locally with a real `A..B` containing a known-bad subject, and the YAML is validated; the end-to-end proof is the next PR with an over-length commit that is not its tip. Branch `batch/168-228-254`.
yonlu closed this issue 2026-09-27 01:45:20 +00:00
gitea-actions Bot removed the
Status
In Progress
 label 2026-09-27 01:45:45 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Labels
Clear labels
Area/Design
Area/Downloads
Area/Explore
Area/Library-UI
Area/Metadata
Area/Packaging
Area/Player
Area/Queue
Area/Settings
Area/Shell-Nav
Compat/Breaking
Breaking change that won't be backward compatible
 Kind/Bug
Something is not working
 Kind/Documentation
Documentation changes
 Kind/Enhancement
Improve existing functionality
 Kind/Feature
New functionality
 Kind/Security
This is security issue
 Kind/Testing
Issue or pull request related to testing
 Platform/Android
Platform/Desktop
Priority
Critical
1
The priority is critical
Priority
High
2
The priority is high
Priority
Medium
3
The priority is medium
Priority
Low
4
The priority is low
Reviewed
Confirmed
1
Issue has been confirmed
Reviewed
Duplicate
2
This issue or pull request already exists
Reviewed
Invalid
3
Invalid issue
Reviewed
Won't Fix
3
This issue won't be fixed
Status
Blocked
1
Something is blocking this issue or pull request
Status
Need More Info
2
Feedback is required to reproduce issue or to continue work
Status
Abandoned
3
Somebody has started to work on this but abandoned work
Status
In Progress
Somebody is actively working on this right now
No labels Area/Packaging Kind/Bug
Priority
Medium
3
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
logan yonlu
No Assignees yonlu
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: yonlu/yellowjacket#254