Restrict Trusted Signing to merged PR release paths in pyRevitCI#3374
Conversation
There was a problem hiding this comment.
PR Summary:
This PR was created by the Copilot agent to fix overly broad Trusted Signing conditions in .github/workflows/main.yml that were causing signing operations to trigger on every ordinary push event. The intent is to restrict signing to only run on merged PRs (WipRun / ReleaseRun), not on plain pushes.
Review Summary:
The PR contains zero file changes. The Copilot agent created an "Initial plan" commit (0 additions, 0 deletions, 0 changed files) and never actually modified .github/workflows/main.yml. The three signing-related steps remain unchanged on the PR branch:
Require Trusted Signing secrets(line 156): still includesgithub.event_name == 'push'in its condition — signing still fires on every push.Sign files with Trusted Signing (DLLs and EXEs)(lines 166–170): conditiongithub.event_name != 'pull_request'is still satisfied by push events.Sign files with Trusted Signing (installers)(lines 191–195): same overly broad condition.- The misleading comment on line 148 ("push run will sign") also still exists.
All three of the PR's acceptance criteria (1–3) are unmet. This PR cannot be merged as-is — the Copilot agent needs to actually commit the fix it described.
The required change for all three if: conditions is:
if: >
github.repository == env.MainRepo &&
(env.WipRun == 'true' || env.ReleaseRun == 'true')Suggestions
- Apply the actual workflow fix by updating all three Trusted Signing
if:conditions to useenv.WipRun == 'true' || env.ReleaseRun == 'true'instead of the current push-inclusive conditions. Apply - Remove the
push:trigger block entirely from the workflow if push events should never produce signing artifacts, to avoid confusion and unnecessary CI runs. Apply
Agent-Logs-Url: https://github.com/pyrevitlabs/pyRevit/sessions/af686bbc-3052-4791-9ca8-d6041e94c924 Co-authored-by: jmcouffin <7872003+jmcouffin@users.noreply.github.com>
pyRevitCI
There was a problem hiding this comment.
Pull request overview
This PR updates the pyRevitCI GitHub Actions workflow to reduce Trusted Signing usage by gating signing-related steps to the workflow’s merged-PR “release paths” (WipRun / ReleaseRun), and updates the nearby comment to reflect the new behavior.
Changes:
- Restricts
Require Trusted Signing secretstogithub.repository == env.MainRepo && (env.WipRun == 'true' || env.ReleaseRun == 'true'). - Restricts both Trusted Signing action steps (binaries + installers) to the same
WipRun/ReleaseRungating. - Updates the comment that previously implied ordinary
pushruns would perform signing.
Agent-Logs-Url: https://github.com/pyrevitlabs/pyRevit/sessions/3d17d591-3773-4d32-bc7e-767b55e11bb7 Co-authored-by: jmcouffin <7872003+jmcouffin@users.noreply.github.com>
Agent-Logs-Url: https://github.com/pyrevitlabs/pyRevit/sessions/8ca3cdde-8b1f-4017-9017-3cf3d9fd1c62 Co-authored-by: jmcouffin <7872003+jmcouffin@users.noreply.github.com>
|
@devloai please review again |
There was a problem hiding this comment.
Re-review Summary
Good progress! The PR is now substantive — Copilot made real changes since my last review. Here's what the current state looks like:
✅ All acceptance criteria met:
Require Trusted Signing secrets—pushandworkflow_dispatchremoved from condition; gated toWipRun || ReleaseRunwith fork guard preserved. ✓Sign files with Trusted Signing (DLLs and EXEs)—WipRun || ReleaseRungate added alongside the existing fork guard. ✓Sign files with Trusted Signing (installers)— Same. ✓Upload Installers— Correctly gated: unsigned_signedartifacts won't be uploaded on non-signing runs. ✓Publish Release— Fork guard added defensively (good hygiene). ✓
Minor nit:
One low-severity comment on the updated comment text (see inline). ReleaseRun already covers workflow_dispatch via line 40, so "manual release dispatches" is implicit — but the phrasing is mildly confusing. Easy one-line fix suggested.
Bottom line:
This PR is ready to merge modulo that cosmetic comment. The logic is sound and all three signing steps are now properly scoped to merged-PR-only contexts. 🎉
Suggestions
- Simplify comment on line 148 to remove 'manual release dispatches' redundancy Apply
Co-authored-by: devloai[bot] <168258904+devloai[bot]@users.noreply.github.com>
| - name: Require Trusted Signing secrets | ||
| if: > | ||
| github.repository == env.MainRepo && | ||
| (github.event_name != 'pull_request' || | ||
| github.event.pull_request.head.repo.full_name == github.repository) && | ||
| (env.WipRun == 'true' || env.ReleaseRun == 'true' || | ||
| github.event_name == 'workflow_dispatch' || | ||
| github.event_name == 'push') | ||
| (env.WipRun == 'true' || env.ReleaseRun == 'true') |
| - name: Publish Release (Release) | ||
| id: publish_release | ||
| if: (env.ReleaseRun == 'true' && github.repository == env.MainRepo) | ||
| if: > | ||
| env.ReleaseRun == 'true' && | ||
| github.repository == env.MainRepo && | ||
| (github.event_name != 'pull_request' || | ||
| github.event.pull_request.head.repo.full_name == github.repository) |
_signedartifact handling for non-signing runsOriginal prompt
Update the GitHub Actions workflow at
.github/workflows/main.ymlinpyrevitlabs/pyRevitso Trusted Signing only runs when pull requests are merged, instead of on ordinary push runs.Context:
pyRevitCItriggers onpull_request(opened,reopened,closed) andpushtodevelopandmaster..github/workflows/main.ymlon branchdevelop.WipRunfor merged PRs intodevelopReleaseRunfor merged PRs intomaster, and alsoworkflow_dispatchonmasterRequire Trusted Signing secretsallowsgithub.event_name == 'push'Sign files with Trusted Signing (DLLs and EXEs)runs for any trusted non-PR eventSign files with Trusted Signing (installers)runs for any trusted non-PR eventRequested behavior:
ReleaseRun.if:conditions to use the existingWipRun/ReleaseRunbooleans rather than rawpushchecks.pushevents.Acceptance criteria:
Require Trusted Signing secretsno longer runs on ordinarypushevents.Sign files with Trusted Signing (DLLs and EXEs)no longer runs on ordinarypushevents.Sign files with Trusted Signing (installers)no longer runs on ordinarypushevents.developand merged PRs intomaster.ReleaseRunalready includes manual release dispatch, do not break that behavior.Please create a pull request with the workflow change.
The following is the prior conversation context from the user's chat exploration (may be truncated):
User: The past month changes generated way to many signing. I'd like the signing tonhappen only when merging the prs
Assistant: Yes — the current workflow still signs on every
push, which is why you’re getting too many signing operations.The fix is to gate both signing steps so they run only for:
develop(WipRun == true)master(ReleaseRun == true)workflow_dispatchonmasterif you still want manual release signingRight now, these conditions are too broad:
Require Trusted Signing secretsincludesgithub.event_name == 'push'Sign files with Trusted Signing ...steps run for any trusted non-PR event, including pushSuggested change in
.github/workflows/main.yml: