Skip to content

Commit 57a2506

Browse files
committed
fix(workflows): address reviewer feedback on closed-PR redirect
- Gate Slack notification on already_posted so oncall is paged only on the first external comment per PR. Subsequent comments don't page — the redirect comment has already directed the commenter to issues. - Add per-PR concurrency group so the marker-comment dedup is race-free (cancel-in-progress: false to ensure later runs still execute against the just-posted marker). - Inline steps.pr_state.outputs.state in the Slack payload directly rather than routing it through env: to avoid step-level evaluation ambiguity. The value is enum-typed (merged|closed) so injection-safe.
1 parent 9223efd commit 57a2506

1 file changed

Lines changed: 17 additions & 6 deletions

File tree

.github/workflows/closed-pr-comment.yml

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,14 +17,21 @@ permissions:
1717
pull-requests: write
1818
issues: read
1919

20+
# Serialize per-PR so the marker-comment dedup is race-free (otherwise two
21+
# rapid-fire comments could both see "no marker" and both post a redirect).
22+
# cancel-in-progress is false so the second run still executes after the
23+
# first finishes — we want to evaluate dedup against the just-posted marker.
24+
concurrency:
25+
group: closed-pr-comment-${{ github.event.issue.number }}
26+
cancel-in-progress: false
27+
2028
jobs:
2129
redirect:
2230
runs-on: ubuntu-latest
2331
# Only fire on PRs (issue_comment fires for issues too) that are closed.
2432
if: >-
25-
github.event.issue.pull_request != null &&
26-
github.event.issue.state == 'closed' &&
27-
github.event.comment.user.type != 'Bot'
33+
github.event.issue.pull_request != null && github.event.issue.state == 'closed' && github.event.comment.user.type
34+
!= 'Bot'
2835
steps:
2936
- name: Check commenter permission
3037
id: perm
@@ -117,7 +124,12 @@ jobs:
117124
core.setOutput('state', mergedAt ? 'merged' : 'closed');
118125
119126
- name: Notify Slack
120-
if: steps.perm.outputs.skip != 'true'
127+
# Page oncall only on the FIRST external comment per PR (gated by
128+
# already_posted). Subsequent comments on the same PR don't page —
129+
# the redirect comment has already told the commenter to open an
130+
# issue, and issues page oncall via the issue path. This bounds
131+
# paging volume regardless of how chatty a thread becomes.
132+
if: steps.perm.outputs.skip != 'true' && steps.existing.outputs.already_posted != 'true'
121133
# Attacker-controlled fields are passed through env: rather than
122134
# interpolated into the YAML payload, to prevent workflow injection.
123135
# Schema is uniform across event types: every workflow sends the
@@ -131,7 +143,6 @@ jobs:
131143
PR_TITLE: ${{ github.event.issue.title }}
132144
PR_URL: ${{ github.event.issue.html_url }}
133145
PR_AUTHOR: ${{ github.event.issue.user.login }}
134-
PR_STATE: ${{ steps.pr_state.outputs.state }}
135146
PR_CLOSED_AT: ${{ github.event.issue.closed_at }}
136147
PR_MERGED_AT: ${{ github.event.issue.pull_request.merged_at }}
137148
COMMENT_ID: ${{ github.event.comment.id }}
@@ -156,7 +167,7 @@ jobs:
156167
pr_title: ${{ toJSON(env.PR_TITLE) }}
157168
pr_url: "${{ env.PR_URL }}"
158169
pr_author: "${{ env.PR_AUTHOR }}"
159-
pr_state: "${{ env.PR_STATE }}"
170+
pr_state: "${{ steps.pr_state.outputs.state }}"
160171
pr_closed_at: "${{ env.PR_CLOSED_AT }}"
161172
pr_merged_at: "${{ env.PR_MERGED_AT }}"
162173
comment_id: "${{ env.COMMENT_ID }}"

0 commit comments

Comments
 (0)