ci: run the metadata pipeline on main, and push with a PAT - #489
Open
Loup-Garou911XD wants to merge 2 commits into
Open
ci: run the metadata pipeline on main, and push with a PAT#489Loup-Garou911XD wants to merge 2 commits into
Loup-Garou911XD wants to merge 2 commits into
Conversation
Since ci.yml pushes with a PAT, its own commits start new workflow runs, so the job needs to skip them. It did that by matching the "[ci] " message prefix, which is unsafe: ci-apply.yml commits "[ci] apply-version-metadata" and "[ci] apply-plugin-metadata-and-formatting" onto the PR BRANCH, so those routinely sit at a PR branch's tip (7d8b18e, 2b1763a). Rebase merge is enabled on this repo, so rebase-merging an ordinary PR makes one of them main's head commit and skips this entire job, the authoritative test_versions run included. That is the worst push to skip: a rebase rewrites the very shas ci-apply.yml stamped into the manifests. The three auto-commit steps now commit as `plugman-ci` and the guard keys on that. Nothing else commits under that identity, so it skips this workflow's own pushes and nothing more; ci-apply.yml's commits (github-actions[bot]) now get a full run when they reach main. Also in ci.yml: - Skip the job on forks. They have no PAT, so they only ever fail at the preflight, and their main is not authoritative for anything. - Record why the missing-PAT preflight fails the whole job rather than letting the tests run. Without a PAT the metadata steps and the suite both still pass against the runner's tree, so the run would go green over a main whose committed manifests were never stamped. No signal beats a false one. - Reset the remote URL once the pushes are done, so the test suite cannot reach a credential that bypasses main's ruleset. Partial only: autopep8 and GitPython are unpinned and already ran with it live. release.yml gets a concurrency group for the same root cause. It filters on paths: [index.json], which is exactly what "[ci] apply-version-metadata" rewrites, so under a PAT that push now starts a release run of its own. Two runs in flight for one release read the same previous tag and both try to create the new one. Serialised, the second sees the published tag and no-ops through versioning_tools.py. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QDSzHqhTmdMMD8f3UF6eNo
There was a problem hiding this comment.
🟡 Changes recommended
The CI workflow contains a non-existent actions/setup-python@v7 reference and the PAT “drop credentials” step doesn’t actually clear checkout’s auth header, leaving elevated credentials available to later steps.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the GitHub Actions workflows to ensure plugin/version metadata is generated and committed on main, and to allow the workflow to push those updates despite branch rules requiring PR-only changes by using a PAT plus a self-trigger guard.
Changes:
- Update
ci.ymlto push metadata/formatting commits usingsecrets.PAT, add a preflight for missing PAT, and guard against re-entry by keying off a dedicated committer identity. - Ensure the CI pipeline runs the plugin metadata application step so manifests stay consistent with plugin bytes.
- Add
concurrencytorelease.ymlto prevent multiple release runs racing whenindex.jsonis rewritten by CI.
File summaries
| File | Description |
|---|---|
| .github/workflows/ci.yml | Switch CI pushes to PAT, add preflight + self-trigger guard, and add credential drop before tests. |
| .github/workflows/release.yml | Serialize release workflow runs to avoid tag/release races caused by CI-triggered index.json updates. |
Review details
Suppressed comments (1)
.github/workflows/ci.yml:113
actions/setup-python@v7is not a valid major version for the official action (latest is v5), so this workflow will fail at runtime when trying to resolve the action ref.
token: ${{ secrets.PAT }}
- name: Set up Python
uses: actions/setup-python@v7
with:
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+207
to
+208
| - name: Drop push credentials | ||
| run: git remote set-url origin "https://github.com/${GITHUB_REPOSITORY}.git" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two commits. The first makes
ci.ymlable to do its job at all; the second fixes problems the first one introduces.fix require pr issue for ci(f4eea3d)ci.ymlnever ranauto_apply_plugin_metadata.py. A push that bumps a plugin'splugmanversion therefore landed the new bytes with no matching manifest entry, leaving the manifest advertising the OLD version'smd5sum. The in-game manager verifies MD5 before installing, so every download of that plugin fails, andtest_latest_versionfails on that run and every later one. That is what8628c97(finder 1.0 to 4.1) did to main; it stayed broken until #485 reverted the change, restoring the old bytes rather than recording the new ones.The commit adds the missing step, plus a
Resolve push basestep so the diff andPLUGMAN_BASE_REFare computed fromgithub.event.beforebefore any auto-commit rewrites HEAD.It also switches the pushes from
GITHUB_TOKENtosecrets.PAT.ci.ymlcurrently cannot push to main at all:That is a protection-rule rejection rather than a permissions one, so no
permissions:value fixes it, andGITHUB_TOKENcannot be given a ruleset bypass (bypass actors are repository roles, teams, installed Apps or Dependabot). The PAT account sits on the bypass list instead.GITHUB_TOKENis dropped tocontents: read.key the self-trigger guard on committer identity(f3575ca)Unlike
GITHUB_TOKEN, a PAT's pushes start new workflow runs, so the job must skip its own commits. The first commit did that by matching the[ci]message prefix. That is unsafe:ci-apply.ymlcommits[ci] apply-version-metadataand[ci] apply-plugin-metadata-and-formattingonto the PR branch, so those routinely sit at a PR branch's tip (7d8b18e,2b1763a). Rebase merge is enabled here, so rebase-merging an ordinary PR makes one of them main's head commit and skips the entire job, including the authoritative stricttest_versionsrun. That is the worst possible push to skip, because a rebase rewrites the very shasci-apply.ymlstamped into the manifests.The three auto-commit steps now commit as
plugman-ciand the guard keys on that. Nothing else uses that identity, so it skips this workflow's own pushes and nothing more;ci-apply.yml's commits reaching main now get a full run. Squash merge was never affected, sinceCOMMIT_OR_PR_TITLEresolves to the PR title on a multi-commit branch.Also in
ci.yml:ci-apply.yml's MERGE GATE comment describes.autopep8andGitPythonare unpinned and already ran with it live.release.ymlgets aconcurrencygroup for the same root cause. It filters onpaths: [index.json], exactly what[ci] apply-version-metadatarewrites, so under a PAT that push now starts a release run of its own. Two runs for one release read the same previous tag and both try to create it. Serialised, the second sees the published tag and no-ops throughversioning_tools.py. Nevercancel-in-progress: a cancelled run can leave a tag with no release attached.Before merging
secrets.PATpredates this work (created 2022-08-31, never rotated, referenced nowhere else). Confirm it is still valid and that its owner can bypass ruleset 22381346. That ruleset's existing bypass isRepositoryRole:5(repository admin), so an admin's token needs no new entry; a non-admin's does.reposcope across every repo its owner can reach.Testing
No test covers workflow files. The plugin-metadata step was validated by replaying five synthetic cases (version bump stamps the manifest and 21 tests pass; no bump fails loudly; non-plugin push no-ops; deletions are filtered out by
--diff-filter=d; a new plugin gets an entry) and eight real historical merges, seven of which were clean no-ops. Both files parse as YAML and the step order is unchanged apart from the additions described above.🤖 Generated with Claude Code
https://claude.ai/code/session_01QDSzHqhTmdMMD8f3UF6eNo