CI: write the changelog entry for pull requests from forks too - #1105
CI: write the changelog entry for pull requests from forks too#1105thc1006 wants to merge 1 commit into
Conversation
GitHub withholds secrets from a `pull_request` run whose head is a fork, so RELEASE_TOKEN arrived empty and the checkout stopped five seconds in with "Input required and not supplied: token". Nothing else refers to the changelog, so the merge completed and the entry was simply absent. The split was clean in both directions across every run still on record: RocketPy-Team#1097, RocketPy-Team#1070 and RocketPy-Team#1052 came from forks and failed; RocketPy-Team#1079, RocketPy-Team#1082 and RocketPy-Team#1056 came from branches here and passed. `pull_request_target` receives the secrets. It is usually the dangerous trigger, but not here: the job checks out RocketPy-Team/RocketPy at develop rather than the pull request, and there is no reference to head.sha, head.ref or head.repo anywhere in the file, so no contributor code runs. Title and body were already being handled as untrusted, passed through the environment and read in Python rather than interpolated into a shell. `github.event.pull_request` keeps the same shape, so the merged guard and PR_NUMBER carry over unchanged. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1105 +/- ##
===========================================
+ Coverage 82.18% 82.55% +0.37%
===========================================
Files 122 128 +6
Lines 16355 16555 +200
===========================================
+ Hits 13441 13667 +226
+ Misses 2914 2888 -26 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Moving this to draft. It does not work, and I would rather say so than have it merged into a state where nothing changes.
RocketPy's default branch is on:
pull_request:
types: [closed]So merging this into There is a second thing I had wrong, and it is the more interesting one. I argued the job runs no contributor code because it checks out It takes a review miss to get there, and someone who can land a change to a CI script has other options. But "we do not check out head.sha" is not the same as "no contributor code runs", and I stated the first as if it settled the second. A version that works needs the trigger on Leaving it in draft rather than closing it, since the issue and the analysis are still worth having attached to something. Happy to close it instead if you would rather. |
|
A measurement on what the bug is currently costing, since I had only described the failure and not its effect. The last six The split is exactly fork versus not, which is the token being withheld and nothing else. The effect is that no fork pull request gets a changelog entry. #1102, #1103, #1104, #1108 and this one all have none, so on today's behaviour they would reach a release unlisted. I have not added entries by hand, since that is the job this workflow exists to do and I would rather not guess at the convention. I also fixed the linking keyword in the description: it said Still a draft for the reason in the description. The workflow is read from the default branch, so landing this on |
Addresses #1101. Not
Closes, since the keyword only fires on a pull request into the default branch and this one targetsdevelop.Pull request type
Current behavior
Populate Changelogfails for every pull request opened from a fork, and only for those. GitHub withholds secrets from apull_requestrun whose head is a fork, sosecrets.RELEASE_TOKENis empty and the first step stops after about five seconds:Nothing else refers to the changelog, so the merge completes and the entry is absent. The split is clean in both directions across every run still on record:
This is not losing history, and I would rather be accurate about the size of it. Both older cases were caught by hand: #1052 merged on 07-11 and its entry was added on 07-12 in
ba9d130; #1070 merged on 07-19 and was picked up the same day ind31822d, under "changelog consolidation" while v1.13.0 was being prepared. What it costs is somebody noticing, once per outside contribution, often not until a release is being assembled.New behavior
pull_request_targetruns in the base repository's context and does receive the secrets.Why that trigger is safe here
It is usually the one to avoid, so it is worth being specific rather than asserting it.
The job never touches the pull request. It checks out
RocketPy-Team/RocketPyatref: developand runs.github/scripts/update_changelog.pyfrom that same trusted tree. There is nohead.sha,head.reforhead.repoanywhere in the file, so nothing from the contributor's branch is fetched or executed.The contributor-controlled values are the title, body and labels, and they were already being treated as untrusted, by the workflow's own comment:
The one interpolation into a shell is
github.event.pull_request.numberin the commit message, which is an integer GitHub assigns.What changes is who can supply that title and body: today only people with write access, afterwards any contributor. The handling was built for untrusted input either way, and the script validates the model's output and falls back to a deterministic insert when it is rejected, so the worst case stays inside
CHANGELOG.md.RELEASE_TOKENis used only byactions/checkoutand is not in the script step's environment.github.event.pull_requestkeeps the same shape under either trigger, so themerged == trueguard andPR_NUMBERcarry over unchanged.If you would rather not use that trigger at all,
on: push: branches: [develop]also receives secrets, at the cost of working out which pull request a push came from.Breaking change
Additional information
Worth saying that this lands on outside contributions specifically, which are the ones where a changelog line does the most good and where the contributor is least likely to notice it went missing.
7f85c07from April is titled "DEV: correct auto-changelog access token", so the token has had attention before. This looks like a different failure, but I have not gone back to check what that one was.I have not run this. A workflow trigger only proves itself once it is on the default path, and the failing half is by definition not reachable from a fork branch.