Skip to content

feat: add reusable PR comment reminder workflow#15

Merged
charly-bg merged 5 commits into
mainfrom
feat/pr-comment-reminder
Apr 3, 2026
Merged

feat: add reusable PR comment reminder workflow#15
charly-bg merged 5 commits into
mainfrom
feat/pr-comment-reminder

Conversation

@dalkia

@dalkia dalkia commented Apr 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Usage example

name: PR Merge Alignment Reminder

on:
  pull_request:
    types: [opened]

jobs:
  reminder:
    uses: decentraland/actions/.github/workflows/pr-comment-reminder.yml@main
    with:
      message: |
        ## 🔗 Merge Alignment Reminder
        If this PR targets `main`, please make sure the corresponding changes are also ready to merge.

🤖 Generated with Claude Code

dalkia and others added 2 commits April 3, 2026 07:37
Adds a workflow_call workflow that posts a configurable markdown
comment on newly opened PRs. Callers pass the message as an input.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@decentraland-bot decentraland-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Nice and clean reusable workflow — the idea of centralizing this in decentraland/actions is solid. The jq --arg usage for safe JSON encoding and env-var binding for MESSAGE are well done ✅

A few things worth addressing:

1. 🔴 curl -s fails silently on HTTP errors

curl -s only suppresses progress output — it does not cause a non-zero exit code on 4xx/5xx responses. If the API returns 403 or 404, the step shows green and no comment is posted. This is the main issue Nacho flagged in the thread.

Fix: Add --fail (or --fail-with-body for better error logs):

curl -sf --show-error -X POST \

Alternatively, capture the status code and fail explicitly.

2. 🟡 Move ${{ github.repository }} to an env var

All other GitHub context values are correctly bound through env:, but github.repository is still interpolated directly in the run: block. While github.repository can't contain shell metacharacters today, it's an inconsistency — and if this pattern gets copied to other contexts (like github.head_ref), it becomes a real injection vector.

env:
  GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
  PR_NUMBER: ${{ github.event.pull_request.number }}
  MESSAGE: ${{ inputs.message }}
  REPO: ${{ github.repository }}    # ← add this
run: |
  curl -sf --show-error -X POST \
    -H "Authorization: Bearer $GH_TOKEN" \
    -H "Accept: application/vnd.github+v3+json" \
    "https://api.github.com/repos/$REPO/issues/$PR_NUMBER/comments" \
    -d "$(jq -n --arg body \"$MESSAGE\" '{body: $body}')"

This way there are zero ${{ }} expressions in the run: block.

3. 🟡 Guard against missing PR number

Since this is a workflow_call, the event context is inherited from the caller. If a caller triggers this from push, schedule, or workflow_dispatch, github.event.pull_request.number is undefined and PR_NUMBER will be empty — causing a silent failure on an invalid API path.

Consider adding a guard:

- name: Validate PR number
  env:
    PR_NUMBER: ${{ github.event.pull_request.number }}
  run: |
    if [ -z "$PR_NUMBER" ]; then
      echo "::error::PR_NUMBER is empty. This workflow must be called from a pull_request event."
      exit 1
    fi

4. 💡 gh CLI alternative (optional)

As Nacho mentioned in the thread — the gh CLI is pre-installed on GitHub runners and handles auth + error reporting automatically:

gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$MESSAGE"

This removes the need for manual curl headers and error handling. Purely a style choice though — the curl + jq approach works fine with the fixes above.

CI Status

✅ All checks passing

Security

No hardcoded secrets. Token permissions are correctly scoped to pull-requests: write. The jq --arg usage properly escapes the message body. With the github.repository env-var fix above, the workflow would have zero script injection surface.


Requested by Ignacio Mazzara via Slack

Comment thread .github/workflows/pr-comment-reminder.yml Outdated
Comment thread .github/workflows/pr-comment-reminder.yml Outdated

@decentraland-bot decentraland-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review: ✅ Approved

Great update — switching from curl to gh pr comment addresses all the previous findings cleanly:

  • curl -s silent failure — no longer applicable; gh CLI exits non-zero on API errors automatically
  • ${{ github.repository }} inline in run: block — no longer needed; gh resolves the repo from context
  • All context expressions routed through env:GH_TOKEN, PR_NUMBER, and MESSAGE are all correctly set via environment variables, eliminating script injection risk in the run: block
  • Minimal permissionspull-requests: write is the narrowest available scope
  • No third-party action dependencies — only the pre-installed gh CLI

One suggestion (non-blocking)

🟡 Missing PR-number guard — since this is a workflow_call, if a caller invokes it from a non-pull_request event (push, schedule, workflow_dispatch), PR_NUMBER will be empty and the step will fail with an unclear error. A simple guard would make the failure message actionable:

run: |
  if [[ -z "$PR_NUMBER" ]]; then
    echo "::error::PR_NUMBER is empty. This workflow must be called from a pull_request event."
    exit 1
  fi
  gh pr comment "$PR_NUMBER" --body "$MESSAGE"

This is optional — callers will presumably always use pull_request triggers, so the risk is low.

Security review

No security issues found. The env: indirection pattern is correctly applied to all GitHub context expressions, preventing shell injection. GITHUB_TOKEN is ephemeral and scoped.

LGTM — clean and minimal. 🚀

Requested by Juan Ignacio Molteni via Slack

@charly-bg
charly-bg merged commit 4152778 into main Apr 3, 2026
1 check passed
@charly-bg
charly-bg deleted the feat/pr-comment-reminder branch April 3, 2026 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants