Upload changelogs, comment-only interaction on fork prs.#140
Conversation
Mpdreamz
left a comment
There was a problem hiding this comment.
Suggested alternative: use docs-builder changelog add --prs instead of artifact harvesting
The fork-PR harvesting approach in this PR works, but I think there's a simpler and more robust path using a docs-builder command that already exists.
The problem with artifact harvesting
The new harvest-fork-prs.js script does a lot of work to safely carry a YAML file from a PR-time workflow run to the merge-time upload run:
- Download a zip artifact from a prior workflow run
- Unzip it onto the runner
- Defend against zip-slip (path traversal in zip entries)
- Refuse symlinks inside the zip
- Read and parse
metadata.json - Verify
pr_numberandhead_shamatch expectations - Validate that
changelog_dirandchangelog_filenameare safe relative paths with no..segments - Copy the YAML into the bundle directory
That's a substantial amount of security-sensitive code whose entire job is to trust something that was generated during the PR lifecycle and stored for up to 90 days. And the 90-day expiry introduces a real failure mode: a fork PR that sits unmerged for 3+ months silently drops its changelog entry on merge (logged as a warning, not a failure).
The simpler path: re-derive at merge time
docs-builder changelog add --prs <N> already does exactly what's needed: given a PR number, it hits the GitHub API, reads the PR's title and labels, applies your docs/changelog.yml label mappings, and writes out the YAML. It reads your config file, so label-to-type/area/product mappings are respected identically to the PR-time generation.
The upload workflow on main already has to:
- Call
GET /repos/{owner}/{repo}/commits/{sha}/pullsto find merged fork PRs (same API call, same logic)
Instead of then fetching an artifact, it could just run:
- name: Add fork-PR changelog entries
run: |
for pr in $FORK_PR_NUMBERS; do
docs-builder changelog add --prs "$pr"
done(Or pass all numbers in one invocation if --prs accepts multiple values, which the docs suggest it does.)
Why this is better
| Artifact harvest (this PR) | changelog add --prs |
|
|---|---|---|
| Artifact storage required | Yes (90-day retention) | No |
| Expiry failure mode | Yes — silent drop after 90 days | No |
| Zip-slip / symlink defense | Required | Not applicable |
| Metadata verification (sha, pr_number) | Required | Not applicable |
| Path traversal validation | Required | Not applicable |
| Source of truth | Artifact generated weeks ago | Live GitHub PR record at merge time |
| New code to maintain | ~250 lines JS + security hardening | ~5 lines shell |
The artifact approach essentially uses the artifact as a trusted courier for data that GitHub already holds and that docs-builder already knows how to fetch. Re-deriving at merge time is simpler, safer, and removes the expiry constraint entirely.
Suggested change
In changelog/upload/action.yml, replace the "Harvest fork-PR changelog artifacts" step with something like:
- name: Add changelog entries for merged fork PRs
env:
PUSHED_SHA: ${{ github.sha }}
with:
github-token: ${{ inputs.github-token }}
run: |
# Find fork PRs merged in this push and generate their changelog entries
# directly from GitHub — no artifact needed
gh api "repos/${{ github.repository }}/commits/${PUSHED_SHA}/pulls" \
--jq '.[] | select(.merged_at != null) | select(.head.repo.full_name != .base.repo.full_name) | .number' \
| xargs -I{} docs-builder changelog add --prs {}(Exact invocation may need adjustment depending on how docs-builder changelog add handles the config path and output directory, but the principle is: look up the merged fork PRs, call changelog add --prs for each one, let the existing upload step pick them up.)
The actions: read permission added to the upload workflow could then be dropped (only pull-requests: read is needed to look up the merged PRs), and harvest-fork-prs.js along with its security-hardening helpers could be deleted entirely.
|
@Mpdreamz Cool. I went with trying to stick with the previous changelog to avoid consistency issues between the "promised" output just prior to the merge in the comment, and the actual contents. But it makes sense to regenerate it, doing it now. |
Mpdreamz
left a comment
There was a problem hiding this comment.
The updated approach using docs-builder changelog add --prs is much cleaner. Thanks for switching it out!
This pull request implements robust support for changelog entries from forked pull requests (PRs), ensuring that changelog entries from forks are reliably harvested and uploaded on merge, even when direct branch commits are not possible. The documentation and workflows are updated to clarify this process and remove dead code related to unsupported push paths. The most important changes are summarized below:
Changelog fork-PR harvesting and upload improvements:
The upload workflow and action (
changelog/upload/action.yml,changelog/upload/README.md) now look up merged PRs for the pushed commit and, for each merged fork PR, harvest the verifiedchangelog-stagingartifact from its most recent successful submit run, validate its metadata, and stage the YAML into the bundle directory for upload. This ensures fork PR changelogs are included even when the entry was never committed to the branch. [1] [2] [3]The submit/evaluate workflow now uploads the changelog artifact with a 90-day retention (was 1 day) to ensure the artifact is available for harvesting when the fork PR merges.
Documentation and workflow permission updates:
The documentation (
changelog/README.md) is extensively updated to describe the new fork PR harvesting process, clarify that fork PRs always use comment-only mode, and warn against using apaths:filter that would suppress the upload workflow for fork PR merges. [1] [2] [3] [4]Workflow permissions are updated to include
actions: readandpull-requests: readto enable artifact listing and PR lookup for fork-PR harvesting. [1] [2]Cleanup and removal of dead code:
GITHUB_TOKEN) are removed from the submit/apply workflow and scripts. The fallback push comment and related environment variables are eliminated. [1] [2] [3] [4]User-facing messaging improvements:
Action documentation additions:
README.mddocumentation for the submit/evaluate and submit/apply actions, describing their inputs, outputs, and usage. [1] [2]