Skip to content

👷 Require a changelog entry when a package's lib changes - #2596

Open
dylanpulver wants to merge 3 commits into
cfug:mainfrom
dylanpulver:ci/require-changelog-entry
Open

dylanpulver wants to merge 3 commits into
cfug:mainfrom
dylanpulver:ci/require-changelog-entry

Conversation

@dylanpulver

@dylanpulver dylanpulver commented Aug 20, 2026 •

Copy link
Copy Markdown

Closes #1662

The only guard today is the manual checkbox from #1679, which that PR called a temporary solution. It misses things: of the last 200 commits on main, 14 changed a package's lib without adding a line to that package's CHANGELOG.md, including #2464 and #2534.

This adds a Check changelog entry workflow and scripts/check_changelog_entry.sh. The script diffs the PR against its base, takes every changed path under <package>/lib/, and requires <package>/CHANGELOG.md to have gained at least one line; a changelog edit that only removes lines does not count. The package root comes from the path rather than a hardcoded list, so a new package is covered the day it is created, and a package with no CHANGELOG.md is exempt, which keeps dio_test and the example apps out of the way. Only paths under lib/ trigger it, so tests, docs, workflows and pubspec.yaml never do.

The escape hatch is a skip-changelog label rather than a magic string in the commit message, because only someone with write access can apply one, so the waiver stays a maintainer decision. The workflow listens to labeled/unlabeled, so applying it re-runs the check without a new push. The label does not exist in this repo yet; its name lives only in SKIP_CHANGELOG_LABEL.

Trigger is pull_request, not pull_request_target, so a fork PR runs in the fork's context with a read-only token: no secret is read, the job requests contents: read and nothing else, and nothing is posted back. No paths filter, on purpose, so it always reports a result and can be made a required check without blocking docs-only PRs. A **/lib/** filter was tried and reverted in review: GitHub leaves a required check skipped by a path filter Pending and blocks the pull request, so filtering trades away the property this is for, and the job is a checkout plus one git diff.

I ran the script over the last 200 commits of main, treating each as a PR, against an independently written Python implementation of the same rule. The two agreed on all 200: 186 pass, 14 fail. #2591 (changed dio/lib, added a changelog line) passes; #2464 (changed dio/lib/src/options.dart, added only tests) fails; #2534, which changed both dio/lib and plugins/web_adapter/lib but updated only the web adapter changelog, fails naming dio alone. I reproduced a GitHub merge ref locally with git merge --no-ff and confirmed it fails on a missing entry, passes once added, passes with the label in the payload, and still fails when the changelog is touched but only loses a line. A label payload with shell metacharacters is parsed as JSON and stays inert. shellcheck clean; zizmor 1.29.0 with the repo's config and --persona=pedantic reports nothing for the new workflow or for .github as a whole.

Unverified: the workflow has never executed on GitHub Actions. Only a real run exercises the event payload, the merge commit actions/checkout leaves at HEAD, the sparse checkout of scripts, and the re-run on a label change; I emulated each locally. The first real evidence is this PR, which changes no package lib and should pass by finding nothing to require. I did not run melos run test or melos run analyze, since no Dart source is touched.

No CHANGELOG.md here, since AGENTS.md scopes that to packages the change touches and this touches none, matching workflow-only PRs such as #2587.

Claude (Opus) was used to design and implement this change and to run the local verification described above. I reviewed the result and own it.

Closes cfug#1662

Co-Authored-By: Claude <noreply@anthropic.com>
@dylanpulver
dylanpulver requested a review from a team as a code owner August 20, 2026 16:32
Comment on lines +8 to +10
on:
pull_request:
types: [ opened, reopened, synchronize, labeled, unlabeled ]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would a path glob filter be more effective here?

A pull request that changes no package `lib/` can never need an entry, so
filtering on paths keeps it from starting a job at all. Across the last 45
merged pull requests this skips 30 of them.

`paths` is matched against the whole pull request rather than the newest
push, so a pull request that touches `lib/` stays in scope while its
changelog entry is added and the check re-runs to pass.

Co-Authored-By: Claude <noreply@anthropic.com>

@AlexV525 AlexV525 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Requesting changes on one point; the rest of the mechanism verified.

The per-package keying is correct: a change under dio/lib requires a line in dio/CHANGELOG.md specifically. Against a byte-identical copy of the script, updating only plugins/web_adapter/CHANGELOG.md while changing dio/lib still fails naming dio, and the #2534 shape reproduces on the real commit (f9705fe → ::error::dio/lib was modified without adding an entry to dio/CHANGELOG.md). An independently written implementation of the same rule over the last 200 first-parent commits of main gives 186 pass / 14 fail, matching your numbers, with #2464, #2534 and #2591 behaving exactly as you described. Nested lib segments, --no-renames, the dio_test/example exemptions and the label payload all behave as documented — a malformed or hostile label payload stays inert and fails closed.

Two things block this as written.

1. The description now contradicts the workflow

.github/workflows/check_changelog_entry.yml:15-16 adds paths: ['**/lib/**'], but the body states:

No paths filter, on purpose, so it always reports a result and can be made a required check without blocking docs-only PRs.

and the file's own comment at lines 10-11 says the opposite: "this check cannot be made a required status check as written." GitHub documents that checks skipped by a path filter stay in a Pending state and that a pull request requiring them is blocked from merging (skipping workflow runs, workflow syntax). dio has no required status checks today, so nothing breaks yet — but the property the description argues for is forfeited, and that paragraph is now wrong.

Either revert the filter (the job is a checkout plus one diff, on PRs that already run the full 3-SDK matrix), or rewrite that paragraph and the "first real evidence" sentence below. Whichever way it goes, please leave a reply here so the reasoning is recorded for the next reader.

2. "The first real evidence is this PR" does not happen

This PR produces no run of the new workflow at all: the only runs on f557189 are CodeQL, Verify packages abilities and CI/CD Security Analysis. A newly added workflow does run on its own PR when its filters allow it — #2587's new cicd_security_analysis.yml ran there as the Scan check (run 31145052489, head_branch=feat-ci-security-analysis). With the filter in place, the first real evidence will be the first post-merge PR that touches a package lib/.

Two one-line bypasses

Both reproduced locally; low severity, cheap to close.

  • scripts/check_changelog_entry.sh:82 — the exemption is read from head, so a PR that deletes <package>/CHANGELOG.md while changing that package's lib/ becomes exempt and passes. Reading it from base (git cat-file -e "${base}:${package}/CHANGELOG.md") closes it; the label covers genuine package removals.
  • scripts/check_changelog_entry.sh:71 — [[ "$added" != "0" ]] also accepts -, which --numstat prints for binary diffs, so appending binary content to a CHANGELOG.md counts as a new entry. [[ "$added" =~ ^[0-9]+$ && "$added" -gt 0 ]] fixes it.

What I verified

  • Byte-identical copy of the script (same sha256 as pr2596:scripts/check_changelog_entry.sh) against a real merge ref, in a sparse-checkout: scripts worktree, through both the BASE_SHA/HEAD_SHA path and the HEAD^1/HEAD^2 fallback: fails with no entry, passes once the entry is added, passes with the label applied.
  • 27-case adversarial matrix: per-package keying, changelog-only deletions, subdirectory changelogs, pubspec.yaml/docs/workflow-only changes, brand-new packages, moves out of lib/.
  • Repo inventory: 10 lib/ directories, 6 changelogs — the exemption set is exactly dio_test, example_dart, example_flutter_app, plugins/native_dio_adapter/example.
  • The action pin 3d3c42e5… is the v7.0.1 tag commit and matches the other workflows; permissions: {} with job-level contents: read, the concurrency group and the runner follow existing conventions; the YAML parses to the expected on.pull_request.paths.

Not verified on my side: shellcheck and zizmor (neither is available to me, and CI has no shellcheck step), so those two claims rest on your local runs. The CI runs on f557189 expired unapproved, so there is no CI signal on this branch either way.

Happy to re-review once one of the two directions for the filter is picked.

Review raised four things.

Reverts the `paths: ['**/lib/**']` filter added in the previous commit. GitHub
leaves a required check skipped by a path filter in a Pending state and blocks
the pull request, so filtering forfeits the ability to make this a required
check, which is the property the design is built around. The job is a checkout
plus one `git diff` on pull requests that already run the three-SDK matrix. The
comment now records that reasoning instead of the opposite.

Reverting it also means the workflow runs on this pull request, so the "first
real evidence is this PR" claim becomes true rather than needing a rewrite.

`--numstat` prints `-` rather than a count for a binary diff, and `-` is not
"0", so appending binary content to a CHANGELOG.md counted as a new entry. The
test is now that the field is a number greater than zero.

The no-changelog exemption was read from `head`, so a pull request that deleted
a package's CHANGELOG.md while changing that package's `lib/` exempted itself.
It is read from `base` now; a genuine package removal is still covered by the
label.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011M5uTyCU4WcNTsPvGrErDo
@dylanpulver

Copy link
Copy Markdown
Author

All four addressed in ce370d5. Thank you for the review, the two bypasses in particular.

1. The filter is reverted, and that was the right call for the reason you gave. A required check skipped by a path filter stays Pending and blocks the pull request, so filtering trades away exactly the property the design is for, and the job is a checkout plus one git diff on pull requests already running the three-SDK matrix. The workflow comment now records that instead of the opposite, and the body says the filter was tried and reverted so the next reader does not have to reconstruct it.

2. Which also fixes the evidence claim rather than needing a rewrite. With no filter the workflow runs on this pull request, so "the first real evidence is this PR" is true again. You were right that it produced nothing on f557189, and the filter was why.

3 and 4. Both bypasses reproduced and closed. Built a throwaway repo in dio's shape and ran the previous script and the new one over the same two commits:

bypass A, change lib and delete the package CHANGELOG.md
  previous script: PASS   (self-exempted)
  new script:      FAIL   (blocked)

bypass B, change lib and append binary content to CHANGELOG.md
  git diff --numstat -> "-  -  dio/CHANGELOG.md"
  previous script: PASS   (the "-" was not "0")
  new script:      FAIL   (blocked)

Fixes are the ones you specified: the exemption reads base rather than head, and the addition count has to be a number greater than zero.

Four controls so the fixes do not over-reach:

genuine text entry added           -> PASS   (unchanged)
package with no CHANGELOG in base  -> PASS   (still exempt, dio_test and the examples)
lib change, no entry, no label     -> FAIL   (unchanged)
same with skip-changelog applied   -> PASS   (label still waives)

Still not verified on my side: shellcheck and zizmor, same as before. CI on this branch has no signal either, so the run on this push is the first real one.

Written with AI assistance (Claude Opus 5). The two tables above are from local runs against a synthetic repo, not against dio's history.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add CI step to ensure a changelog entry is added

2 participants