Skip to content

Commit fefb6cb

Browse files
GabrielBBaldezGabrielBBaldez
committed
fix(ci): thank-you comment needs pull-requests: write, not issues: write
The POST goes to /repos/:owner/:repo/issues/:number/comments, so `issues: write` is the obvious reading of what it needs, and it is wrong: GitHub scopes that endpoint by what the number points at, and for a pull request the permission checked is pull-requests. `pull-requests: read` in the block was therefore the denial, while `issues: write` was granted and never consulted. An explicit permissions block replaces the default rather than adding to it, so the repository being on "Read and write permissions" could not compensate. Every run was green throughout — a 403 here only raises a ::warning::, and nobody reads a warning on a green run. Verified in stacktale-intellij: with the permission corrected, a dispatch against the PR that first surfaced this posted the comment. Also moves the trigger to push on main, matching that repo. Treat it as a preference rather than a fix: a push is plainly not fork-triggered, which takes the question of what token a fork PR receives off the table. pull_request_target was never tested with the permission right.
1 parent e31fe5a commit fefb6cb

1 file changed

Lines changed: 59 additions & 24 deletions

File tree

‎.github/workflows/first-contribution.yml‎

Lines changed: 59 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,48 +1,83 @@
11
name: First contribution
22

3-
# Thanks someone the first time a PR of theirs is merged. On merge rather than on open:
4-
# at that point they've actually given something, so it reads as thanks instead of a pitch.
3+
# Thanks someone the first time a PR of theirs is merged. On merge rather than on open: at
4+
# that point they've actually given something, so it reads as thanks instead of a pitch.
55
#
6-
# Note on the check: author_association is NOT usable here. Merging a PR promotes its author
7-
# from FIRST_TIME_CONTRIBUTOR to CONTRIBUTOR, and the payload delivered with the `closed`
8-
# event already carries the new value — so a condition on it never matches. Counting the
9-
# author's merged PRs is the signal that survives that.
6+
# On `push` to main rather than `pull_request_target: closed`. The 403s that prompted the
7+
# change turned out to be the permissions block below, not the trigger — so treat this as a
8+
# preference, not a fix: a push to main is plainly not fork-triggered, which takes the whole
9+
# question of what token a fork PR gets off the table. `pull_request_target` would very
10+
# possibly work now too; it was never tested with the permission right, because the
11+
# permission was wrong the entire time.
1012
#
11-
# pull_request_target is required to comment on a fork's PR — it runs with the base repo's
12-
# permissions. It therefore MUST NOT check out or execute the PR's code; this job only
13-
# posts a comment, and has no checkout step for that reason.
13+
# Every run before that fix was green. A 403 here only raises a ::warning::, and nobody reads
14+
# a warning on a green run — which is why this went unnoticed through several merges.
15+
#
16+
# Note on the check: author_association is NOT usable here either. Merging a PR promotes its
17+
# author from FIRST_TIME_CONTRIBUTOR to CONTRIBUTOR before the event is delivered, so a
18+
# condition on it never matches. Counting the author's merged PRs survives that.
1419
on:
15-
pull_request_target:
16-
types: [closed]
20+
push:
21+
branches: [main]
22+
workflow_dispatch:
23+
inputs:
24+
pr:
25+
description: "PR number to thank for (testing; skips the push-derived lookup)"
26+
required: true
1727

1828
permissions:
19-
pull-requests: read
20-
issues: write
29+
contents: read
30+
# write, and it must be pull-requests rather than issues. The comment goes through
31+
# POST /repos/:owner/:repo/issues/:number/comments, so `issues: write` is the obvious
32+
# reading and it is wrong: GitHub scopes that endpoint by what the number points at, and
33+
# for a pull request the permission checked is pull-requests. Declaring `pull-requests:
34+
# read` here was the denial — an explicit permissions block is absolute, so it capped the
35+
# very thing being asked for while `issues: write` was granted and never consulted.
36+
pull-requests: write
2137

2238
jobs:
2339
thanks:
24-
if: github.event.pull_request.merged == true && github.event.pull_request.user.type != 'Bot'
2540
runs-on: ubuntu-latest
2641
steps:
2742
- env:
2843
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
2944
REPO: ${{ github.repository }}
30-
PR: ${{ github.event.pull_request.number }}
31-
AUTHOR: ${{ github.event.pull_request.user.login }}
45+
SHA: ${{ github.sha }}
46+
MANUAL_PR: ${{ inputs.pr }}
3247
run: |
3348
set -euo pipefail
3449
50+
if [ -n "${MANUAL_PR:-}" ]; then
51+
pr="$MANUAL_PR"
52+
else
53+
# Whatever PR this commit came from, however it was merged. Reading the API
54+
# rather than parsing the commit subject: squash writes "(#12)", a merge commit
55+
# writes "Merge pull request #12", and a rebase merge writes neither.
56+
pr=$(gh api "repos/$REPO/commits/$SHA/pulls" --jq '.[0].number // empty')
57+
fi
58+
if [ -z "$pr" ]; then
59+
echo "No pull request behind this commit — direct push."
60+
exit 0
61+
fi
62+
63+
author=$(gh api "repos/$REPO/pulls/$pr" --jq '.user.login')
64+
type=$(gh api "repos/$REPO/pulls/$pr" --jq '.user.type')
65+
if [ "$type" = "Bot" ]; then
66+
echo "$author is a bot."
67+
exit 0
68+
fi
69+
3570
merged=$(gh api --paginate "repos/$REPO/pulls?state=closed&per_page=100" \
36-
--jq ".[] | select(.user.login==\"$AUTHOR\" and .merged_at != null) | .number" | wc -l)
37-
echo "$AUTHOR has $merged merged PR(s) here."
71+
--jq ".[] | select(.user.login==\"$author\" and .merged_at != null) | .number" | wc -l)
72+
echo "$author has $merged merged PR(s) here."
3873
if [ "$merged" -ne 1 ]; then
3974
echo "Not their first — nothing to say."
4075
exit 0
4176
fi
4277
4378
body="$RUNNER_TEMP/thanks.md"
4479
cat > "$body" <<EOF
45-
Merged — thanks @$AUTHOR, that's your first one here.
80+
Merged — thanks @$author, that's your first one here.
4681
4782
If the project turned out to be useful to you, a ⭐ genuinely helps: stacktale is
4883
new, and stars are most of what decides whether anyone else finds it.
@@ -51,9 +86,9 @@ jobs:
5186
list is kept honest, and each one names the files to touch and how to verify.
5287
EOF
5388
54-
# An org policy can force workflow tokens to read-only, which makes this POST 403.
55-
# Don't fail the run over a thank-you note — a red X on every merged PR is worse
56-
# than a missing comment. Switch it on at Settings > Actions > Workflow permissions.
57-
if ! gh api --method POST "repos/$REPO/issues/$PR/comments" -F body=@"$body"; then
58-
echo "::warning::could not comment — the workflow token is read-only for this org"
89+
# Still tolerated rather than fatal: a red X on main over a thank-you note is worse
90+
# than a missing note. The ::warning:: is what makes it visible instead of silent.
91+
if ! gh api --method POST "repos/$REPO/issues/$pr/comments" -F body=@"$body"; then
92+
echo "::warning::could not comment — check the permissions block above and"
93+
echo "::warning::Settings > Actions > Workflow permissions"
5994
fi

0 commit comments

Comments
 (0)