chore: Prevent PRs from touching dist folder #1056

Activity

1. brunoborges

   changed the title [-]Prevent PRs from touching dist folder[/-] [+]Prevent PRs from modifying the dist/ folder and define when to rebuild[/+] on Jun 23, 2026
2. brunoborges commented on Jun 23, 2026

   

   brunoborges

   on Jun 23, 2026

   ContributorAuthor

   More actions

   Policy options for triggering the dist/ build — pro/con

   Note: the repo already has check-dist.yml, which validates that the committed dist/ matches a fresh build of the source. The options below are about when/how the build is produced.

   Option A — Status quo: contributor builds & commits dist/, CI validates

   Pros

   • Already implemented — zero migration cost.
   • main is always immediately runnable as an action (Actions consumes the committed dist/, not source).
   • Tamper-evident: reviewer + CI can diff dist/ against source; supply-chain integrity is verifiable in the PR.
   • No bot write permissions needed.

   Cons

   • Noisy minified diffs clutter review and blame.
   • Frequent merge conflicts in dist/ across concurrent PRs.
   • Friction for contributors who forget to rebuild → red CI and back-and-forth.
   • Detects drift but doesn't prevent hand-edits to dist/.

   Option B — CI auto-builds and commits dist/ back to the PR branch (bot commit)

   Pros

   • Contributors never touch dist/ — directly satisfies the "prevent PRs from touching dist" goal.
   • main stays immediately runnable.
   • Eliminates "you forgot to rebuild" churn.

   Cons

   • Security: requires write-back to PR branches. pull_request_target / pushing to forks is a known privilege-escalation vector; building untrusted PR code with elevated tokens is risky.
   • Pushing to forks from upstream needs a PAT/app token; permissions are awkward.
   • Bot commits can race with contributor pushes and trigger re-run loops.
   • Still large diffs, just authored by a bot — and harder to attribute for supply-chain trust.

   Option C — Build only at release time (remove dist/ from main)

   Pros

   • Cleanest main: no generated artifacts, no diff noise, no conflicts.
   • Fully prevents PRs from touching dist/ (it isn't there).
   • Build provenance centralized in one trusted, audited release workflow.

   Cons

   • Breaks @main/SHA pinning: users who pin to a branch or commit would get a non-runnable action. Major compatibility risk for a popular action.
   • e2e/branch tests that reference the action by ref need a build step everywhere.
   • More complex release tooling; release becomes a single point of failure.
   • Requires reworking the release npm script and release-new-action-version.yml.

   Option D — Build on merge to main (post-merge bot commit)

   Pros

   • Clean PRs (no dist/) while main still ships a runnable dist/.
   • @main and tags keep working.
   • Build runs on trusted main code, not untrusted PR code → safer than Option B.

   Cons

   • Interleaves bot "rebuild" commits into main history.
   • Brief window where merged main is stale until the bot commit lands (can break a SHA pinned to the merge commit).
   • Needs bot push access to protected main (app token / branch-protection exception).
   • Two-step merge; a failed post-merge build leaves main inconsistent.

   ---

   Recommendation

   For a high-traffic action where users pin to branches/SHAs, keeping dist/ on main rules out Option C. Strongest combination:

   • Keep Option A's check-dist validation (integrity guarantee), plus
   • Add a CODEOWNERS / branch-protection rule or a labeler to flag PRs touching dist/ (satisfies the "prevent hand-edits" goal), and optionally
   • Layer Option D (post-merge rebuild) later if PR diff-noise becomes the dominant pain.

   Avoid Option B unless the build can be isolated with a hardened, least-privilege token — the pull_request_target write-back risk outweighs the convenience.
3. brunoborges

   changed the title [-]Prevent PRs from modifying the dist/ folder and define when to rebuild[/-] [+]chore: Prevent PRs from touching dist folder[/+] on Jun 23, 2026
4. brunoborges

   added the

   Task

   issue type on Jun 23, 2026
5. brunoborges

   added

   maintenanceInternal refactors/chore workInternal refactors/chore work

   on Jun 23, 2026
6. brunoborges commented on Jun 25, 2026

   

   brunoborges

   on Jun 25, 2026

   ContributorAuthor

   More actions

   Proposed approach — Option E: keep main dist-free, build dist/ only at release time (tag-only dist/)

   This is a refinement of Option C that avoids a long-lived release branch. The idea: main never contains dist/. The release workflow builds dist/, commits it as a child of the released source commit, and the version tags point at that build commit. main's branch pointer never includes a dist/ commit, so PRs can never touch it.

   Mechanism

   1. Remove dist/ from main and add it to .gitignore.
   2. On release, the workflow:
      • checks out the source commit being released,
      • runs npm ci && npm run build,
      • commits the freshly built dist/ (git add -f dist/) as a single commit on top of that source,
      • tags that build commit as vX.Y.Z, then moves the major tag (v5) to it.
   3. Consumers continue to use actions/setup-java@v5 / @vX.Y.Z / a tag SHA — all of which resolve to a commit that does contain a runnable dist/.

   What keeps working

   • @v5, @vX.Y.Z, and SHA-pins to a tag commit — these are the documented, recommended pins (the README only advertises @v5).
   • Per-release integrity: check-dist validation moves to release time (verify the committed dist/ matches a fresh build before tagging).

   What breaks / must change (costs)

   1. e2e workflows — they reference the action as uses: ./, which consumes the checked-out dist/. There are ~30 usages across 5 files (e2e-versions.yml, e2e-cache.yml, e2e-cache-dependency-path.yml, e2e-local-file.yml, e2e-publishing.yml). Each job needs an injected npm ci && npm run build step before uses: ./ (adds ~1 min/job).
   2. @main and SHA-to-main pins break for any external user pinning to the branch (discouraged, but some do). Tag-based pins are unaffected; the README does not advertise @main.
   3. Release workflow rework — actions/publish-action@v0.4.0 only moves tags; it does not build. The release pipeline (release-new-action-version.yml) must gain a build → commit dist/ → tag step, and check-dist per-PR tamper-evidence (inline source↔dist diff in the PR) is replaced by a release-time check.
   4. Governance / convention — this deviates from the actions/* norm: actions/checkout, actions/setup-node, and actions/setup-python all keep dist/ committed in main. The @actions/setup-actions-team should sign off, since it changes release provenance for a high-traffic official action.

   Pros

   • Clean main: no minified diffs, no dist/ merge conflicts, PRs physically cannot touch dist/.
   • Build provenance centralized in one trusted, audited release workflow (built from main source, never from untrusted PR code → avoids the Option B pull_request_target write-back risk).
   • No bot writes to PR branches and no bot commits interleaved into main history (unlike Option D).

   Cons / risks

   • Breaks @main / main-SHA pinning (compatibility event).
   • Every e2e job must build first (slower CI, more workflow boilerplate).
   • Loss of inline per-PR dist/ review; integrity shifts entirely to the release workflow, making release a single point of failure for runnability.
   • One-time migration + release-tooling rewrite.

   Comparison to the earlier options

   • vs C (release branch): same "dist-free main" benefit, but no long-lived release branch to maintain; tags carry the artifact instead.
   • vs D (post-merge rebuild): D keeps @main working and needs no e2e changes, but interleaves bot commits into main and leaves a brief stale-main window. Option E gives the cleanest history at the cost of @main runnability.

   Recommendation

   Feasible and the cleanest "dist-free main" variant, but it's a compatibility + governance event for a popular action. Suggest the team decide between:

   • Status quo + chore: enforce pre-PR validation (aggregate scripts, git hooks, PR checklist) #1061 hooks + check-dist + CODEOWNERS (lowest risk, already in place), or
   • Option D (clean PRs, @main still works), or
   • Option E (cleanest main, accept @main/SHA-pin breakage and e2e rewrite).

   No implementation here — filing this for discussion. If we choose E or D, next step is a prototype branch + draft PR.
7. brunoborges commented on Jul 16, 2026

   

   brunoborges

   on Jul 16, 2026

   ContributorAuthor

   More actions

   None of the options are ideal. We will continue with status quo.
8. brunoborges

   closed this as not plannedon Jul 16, 2026