fix: forked plugin publish workflow#2014
Merged
Merged
Conversation
elibosley
changed the title
Purpose of the change: - Allow forked pull request builds to publish preview plugins through a trusted workflow path. How behavior was before: - The pull_request build attempted to upload directly to Cloudflare R2 from the reusable plugin build workflow. - Forked builds could not access Cloudflare secrets, leaving --endpoint-url empty and causing the aws command to fail. Why that was a problem: - Network/plugin preview artifacts were built but never uploaded to Cloudflare for forked PRs. What the new change accomplishes: - Pull request builds upload plugin files as GitHub artifacts only. - A workflow_run job running with base repository permissions downloads, validates, uploads, and comments the preview URL. How it works: - The direct Cloudflare upload step is skipped for pull_request events. - The new upload-pr-plugin workflow finds the successful CI run artifact, validates .plg/.txz contents, syncs them to the PR preview path, and posts a sticky PR comment. - Direct push uploads now quote and validate Cloudflare endpoint and bucket inputs.
elibosley
force-pushed
the
codex/fix-forked-plugin-publish
branch
from
565556c to
4a8cdf0
Compare
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughRefactors build-plugin uploads to use optional Cloudflare inputs and env vars, adds upload-pr-plugin.yml to validate and publish PR build artifacts to Cloudflare R2 with PR preview comments, and updates push-staging to fetch/modify/upload PR redirect plugins from R2. ChangesPlugin Distribution Workflows
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Purpose of the change: - Make the merged-PR plugin closeout workflow reliable for forked and non-plugin PRs. How behavior was before: - The workflow ran on pull_request closed events and tried to use Cloudflare secrets directly. - It failed when no successful plugin artifact existed for the PR. - Fork-originated runs had no secret source, so any later R2 operation would fail with empty Cloudflare values. Why that was a problem: - Closed or merged PRs produced noisy workflow failures even when there was no PR plugin to close out. - Forked PR plugin redirects could not safely reach Cloudflare using the pull_request event context. What the new change accomplishes: - Runs merged-PR closeout from pull_request_target so trusted workflow code can access repository secrets. - Checks for the existing PR plugin in Cloudflare R2 and exits successfully when there is nothing to close out. - Rewrites and uploads the staging redirect only when an actual PR plugin exists. How it works: - The workflow validates the PR number and expected R2 key format. - It downloads the existing PR plugin from R2, bumps its version, changes the plugin URL to staging, clears old PR artifacts, then uploads the redirect plugin. - R2 commands now use quoted endpoint values, isolated AWS config, and explicit secret validation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/upload-pr-plugin.yml:
- Line 31: Replace mutable action tags with immutable commit SHAs: update the
uses entry "actions/github-script@v7" to the corresponding full commit SHA
(e.g., "actions/github-script@<commit-sha>") and do the same for other uses
entries mentioned (lines referenced include the other occurrences like the ones
currently using "`@v2`" or "`@v7`"); find the latest tested commit SHAs from each
action's GitHub repo and pin them so the workflow uses the fixed commit SHAs
instead of mutable tags.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9dd213ff-e1a1-45c0-811e-ef518dd2e634
📒 Files selected for processing (2)
.github/workflows/build-plugin.yml.github/workflows/upload-pr-plugin.yml
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a8cdf0fe7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/push-staging-pr-on-close.yml (1)
122-122: 💤 Low valueNote:
.*?in sed is not non-greedy.In sed extended regex,
?is a quantifier (0 or 1 occurrences), not a non-greedy modifier. The.*?here is equivalent to.*followed by an optional empty string, which is just.*. The code works because entity declarations typically have only one">per line, but the?is misleading.Consider removing the
?for clarity, or if non-greedy is truly needed, use a negated character class:Suggested clarification
- sed -i -E "s#(<!ENTITY plugin_url \").*?(\">)#\1${url}\2#g" "${plgfile}" || exit 1 + sed -i -E "s#(<!ENTITY plugin_url \")[^\"]*(\">)#\1${url}\2#g" "${plgfile}" || exit 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/push-staging-pr-on-close.yml at line 122, The sed regex uses ".*?" which is not a non-greedy operator in sed; update the expression in the sed command that targets "(<!ENTITY plugin_url \").*?(\">)" so it no longer contains the misleading "?" — either remove the "?" (i.e. use "(<!ENTITY plugin_url \").*(\">)") or, to enforce non-greedy matching, replace ".*?" with a negated character class like "[^"]*" so it becomes "(<!ENTITY plugin_url \")[^"]*(\">)"; modify the sed invocation that writes to "${plgfile}" and continues to substitute with "${url}" accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/push-staging-pr-on-close.yml:
- Around line 39-50: The workflow currently interpolates the user-provided
inputs.pr_number directly into the shell which can lead to template injection;
instead pass the input via an environment variable and then read that env var
inside the script (avoid direct use of `${{ inputs.pr_number }}` in the script
body). Concretely: set an env var (e.g., PR_INPUT) to `inputs.pr_number` in the
step's env context, then inside the script assign PR_NUMBER from that env var
(or from github.event.pull_request.number when appropriate), keep the numeric
validation on PR_NUMBER (the existing regex check) and write the validated
PR_NUMBER to GITHUB_OUTPUT; ensure all expansions are quoted to prevent
word-splitting or command injection when assigning or echoing PR_NUMBER.
---
Nitpick comments:
In @.github/workflows/push-staging-pr-on-close.yml:
- Line 122: The sed regex uses ".*?" which is not a non-greedy operator in sed;
update the expression in the sed command that targets "(<!ENTITY plugin_url
\").*?(\">)" so it no longer contains the misleading "?" — either remove the "?"
(i.e. use "(<!ENTITY plugin_url \").*(\">)") or, to enforce non-greedy matching,
replace ".*?" with a negated character class like "[^"]*" so it becomes
"(<!ENTITY plugin_url \")[^"]*(\">)"; modify the sed invocation that writes to
"${plgfile}" and continues to substitute with "${url}" accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6b0cd8c1-7b7b-4ce8-8ed5-04cec08f6526
📒 Files selected for processing (1)
.github/workflows/push-staging-pr-on-close.yml
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2014 +/- ##
=======================================
Coverage 52.68% 52.69%
=======================================
Files 1033 1033
Lines 71688 71688
Branches 8211 8205 -6
=======================================
+ Hits 37771 37773 +2
+ Misses 33791 33789 -2
Partials 126 126 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Purpose of the change: - Address CodeRabbit review feedback on the forked plugin publish and PR plugin closeout workflows. How behavior was before: - New workflow actions used mutable tags. - workflow_run upload concurrency could fall back to a non-unique branch name for forked PRs. - workflow_dispatch input was interpolated directly into the closeout shell script. - The closeout sed expression used a misleading non-greedy-looking pattern. Why that was a problem: - Mutable actions and direct input interpolation are avoidable security risks in workflows that handle repository secrets. - Non-unique fallback concurrency could cancel unrelated forked upload jobs. - The sed pattern was confusing and could invite incorrect future edits. What the new change accomplishes: - Pins the new workflow actions to immutable SHAs. - Uses workflow_run.id as the upload concurrency fallback. - Passes workflow_dispatch PR input through step env before shell validation. - Uses a quoted-string character class for plugin_url rewriting. How it works: - GitHub action refs are replaced with resolved 40-character SHAs. - The PR number selector reads trusted shell variables and keeps numeric validation. - The targeted workflow regexes and YAML parse checks now pass.
Purpose of the change: - Preserve the CodeRabbit fix-loop checklist for PR review traceability. How behavior was before: - The checklist existed only as a local untracked file. Why that was a problem: - Reviewers could not see the item-by-item evidence for resolved CodeRabbit feedback. What the new change accomplishes: - Adds the completed WIP checklist under .codex for the PR branch. How it works: - Records the reviewed items, statuses, validation commands, final unresolved-thread check, and resolved thread IDs.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/push-staging-pr-on-close.yml (1)
122-126:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail if the
.plgrewrite does not actually happen.
sed -istill exits 0 when the target entity is missing, and this step never enables fail-fast shell options. A malformed plugin fetched from R2 would therefore be re-uploaded unchanged while the workflow reports a successful redirect. Addset -Eeuo pipefailhere and verify that both theversionandplugin_urlentities exist and were rewritten before uploading.Suggested hardening
- name: Update Downloaded Plugin to Redirect to Staging if: steps.download_pr_plugin.outputs.found == 'true' run: | + set -Eeuo pipefail + IFS=$'\n\t' + # Find the .plg file in the downloaded artifact plgfile="pr-release/dynamix.unraid.net.plg" if [ ! -f "$plgfile" ]; then echo "ERROR: .plg file not found in pr-release/" ls -la pr-release/ exit 1 fi + grep -q '<!ENTITY version "' "$plgfile" + grep -q '<!ENTITY plugin_url "' "$plgfile" + echo "Found plugin file: $plgfile" # Get current version and bump it with current timestamp current_version=$(grep '<!ENTITY version' "${plgfile}" | sed -E 's/.*"(.*)".*/\1/') @@ # Change the plugin url to point to staging - users will switch to staging on next update url="https://preview.dl.unraid.net/unraid-api/dynamix.unraid.net.plg" sed -i -E "s#(<!ENTITY plugin_url \")[^\"]*(\">)#\1${url}\2#g" "${plgfile}" || exit 1 + + grep -q "<!ENTITY version \"${new_version}\">" "$plgfile" + grep -q "<!ENTITY plugin_url \"${url}\">" "$plgfile"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/push-staging-pr-on-close.yml around lines 122 - 126, Add robust shell failure handling and explicit verification around the .plg rewrites: enable strict mode at the start of this step with set -Eeuo pipefail, run the sed substitutions that update the <!ENTITY version> (using new_version and plgfile) and <!ENTITY plugin_url> (using url and plgfile) as currently written, then confirm the replacements actually occurred by checking the file contains the expected new_version and the staging URL (or that sed reported changes) before proceeding to upload; if either check fails, exit non‑zero so the workflow fails fast.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/push-staging-pr-on-close.yml:
- Around line 122-126: Add robust shell failure handling and explicit
verification around the .plg rewrites: enable strict mode at the start of this
step with set -Eeuo pipefail, run the sed substitutions that update the <!ENTITY
version> (using new_version and plgfile) and <!ENTITY plugin_url> (using url and
plgfile) as currently written, then confirm the replacements actually occurred
by checking the file contains the expected new_version and the staging URL (or
that sed reported changes) before proceeding to upload; if either check fails,
exit non‑zero so the workflow fails fast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 16546901-1ae4-4448-a4e7-53382b9f6cb0
📒 Files selected for processing (2)
.github/workflows/push-staging-pr-on-close.yml.github/workflows/upload-pr-plugin.yml
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
1 participant
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.
Summary
pull_requestplugin builds where fork secrets are unavailableworkflow_runuploader that downloads the plugin artifact, validates.plg/.txzcontents, syncs to R2, and comments the preview URLpull_request_targetcontext, reads the existing PR plugin from R2, and skips successfully when there is nothing to close outworkflow_dispatchinput interpolation, fixing upload concurrency fallback, and clarifying the close-outsedpatternRoot Cause
Forked PR builds cannot access the Cloudflare R2 secrets used by the reusable plugin build workflow. The AWS CLI step still ran, so
${{ secrets.CF_ENDPOINT }}expanded to an empty value andaws s3 syncfailed withargument --endpoint-url: expected one argument.The PR close-out workflow had a separate reliability issue: it tried to download a successful
main.ymlplugin artifact on every merged PR and failed if no artifact existed. It also ran onpull_request, so fork-originated runs hadSecret source: Noneand would fail later R2 operations even when an artifact was present.Validation
ruby -e 'require "yaml"; ARGV.each { |f| YAML.load_file(f); puts "ok #{f}" }' .github/workflows/upload-pr-plugin.yml .github/workflows/push-staging-pr-on-close.ymlrg -nP '^\s*uses:\s*[^@]+@(?!(?:[a-f0-9]{40})$).+' .github/workflows/upload-pr-plugin.yml .github/workflows/push-staging-pr-on-close.ymlreturned no matchesrg -n '\.\*\?|\$\{\{ inputs\.pr_number \}\}' .github/workflows/push-staging-pr-on-close.ymlonly reports the safe stepenvassignment forINPUT_PR_NUMBERgit diff --checkcoderabbit-review-data final 2014returnedunresolved_coderabbit_threads 0Replace PR Plugin with Staging Redirect on Mergeruns; latest failure wasno matching workflow run found with any artifacts?GitHub Actions still needs to run the full fork publish and merged-PR close-out handoffs in CI.
Summary by CodeRabbit
New Features
Chores
Documentation