Repository navigation
👷 Require a changelog entry when a package's lib changes - #2596
dylanpulver wants to merge 2 commits into
Conversation
Closes cfug#1662 Co-Authored-By: Claude <noreply@anthropic.com>
| on: | ||
| pull_request: | ||
| types: [ opened, reopened, synchronize, labeled, unlabeled ] |
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
pathsfilter, 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 fromhead, so a PR that deletes<package>/CHANGELOG.mdwhile changing that package'slib/becomes exempt and passes. Reading it frombase(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--numstatprints for binary diffs, so appending binary content to aCHANGELOG.mdcounts 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 asparse-checkout: scriptsworktree, through both theBASE_SHA/HEAD_SHApath and theHEAD^1/HEAD^2fallback: 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 oflib/. - Repo inventory: 10
lib/directories, 6 changelogs — the exemption set is exactlydio_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-levelcontents: read, the concurrency group and the runner follow existing conventions; the YAML parses to the expectedon.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.
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'slibwithout adding a line to that package'sCHANGELOG.md, including #2464 and #2534.This adds a
Check changelog entryworkflow andscripts/check_changelog_entry.sh. The script diffs the PR against its base, takes every changed path under<package>/lib/, and requires<package>/CHANGELOG.mdto 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 noCHANGELOG.mdis exempt, which keepsdio_testand the example apps out of the way. Only paths underlib/trigger it, so tests, docs, workflows andpubspec.yamlnever do.The escape hatch is a
skip-changeloglabel 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 tolabeled/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 inSKIP_CHANGELOG_LABEL.Trigger is
pull_request, notpull_request_target, so a fork PR runs in the fork's context with a read-only token: no secret is read, the job requestscontents: readand nothing else, and nothing is posted back. Nopathsfilter, on purpose, so it always reports a result and can be made a required check without blocking docs-only PRs.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 (changeddio/lib, added a changelog line) passes; #2464 (changeddio/lib/src/options.dart, added only tests) fails; #2534, which changed bothdio/libandplugins/web_adapter/libbut updated only the web adapter changelog, fails namingdioalone. I reproduced a GitHub merge ref locally withgit merge --no-ffand 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.shellcheckclean;zizmor1.29.0 with the repo's config and--persona=pedanticreports nothing for the new workflow or for.githubas a whole.Unverified: the workflow has never executed on GitHub Actions. Only a real run exercises the event payload, the merge commit
actions/checkoutleaves atHEAD, the sparse checkout ofscripts, and the re-run on a label change; I emulated each locally. The first real evidence is this PR, which changes no packageliband should pass by finding nothing to require. I did not runmelos run testormelos run analyze, since no Dart source is touched.No
CHANGELOG.mdhere, 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.