feat: implement issue #538 — Title: pr-review-mention: kill the self-trigger — bot's own mention-ack re-fires the listener

.github/workflows/pr-review-mention-reusable.yml

@@ -40,20 +40,29 @@ jobs:
     # For comment events we also gate at the job level on author_association so
     # that the job (and its secrets) never start for external/untrusted actors.
     #
-    # Recursion guard: the ack comment this job posts itself contains
+    # Recursion guard (#538): the ack comment this job posts contains
     # '@donpetry-bot' and is posted via a PAT (so it DOES re-trigger workflows,
     # unlike GITHUB_TOKEN), which without this guard self-loops until GitHub's
-    # 2500-comment cap. Skip any comment carrying our own ack marker. We match on
-    # the marker (not the bot actor) because donpetry-bot is also a human account
-    # whose own @-mention review requests must still fire.
+    # 2500-comment cap (.github-private#860: 1,481 identical acks over ~4.5h).
+    # A comment the agent posts about its own work must never re-trigger the
+    # agent, so we exclude on TWO independent axes, belt-and-suspenders:
+    #   1. the bot actor — any comment authored by donpetry-bot is skipped, and
+    #   2. the marker — any comment carrying an '<!-- pr-review-agent' marker is
+    #      skipped (broadened from the old 'pr-review-agent mention-ack' string
+    #      so EVERY agent-posted comment, not just the ack, is caught).
+    # The earlier design matched the marker alone to keep donpetry-bot's own
+    # @-mention review requests working, but #860 showed the self-trigger risk
+    # outweighs that convenience; the bot can still request a review via the
+    # review_requested (assign-reviewer) path above.
     if: |
       (github.event_name == 'pull_request' &&
        github.event.requested_reviewer != null &&
        github.event.requested_reviewer.login == 'donpetry-bot' &&
        github.event.pull_request.head.repo.full_name == github.repository) ||
       (github.event_name != 'pull_request' &&
        contains(github.event.comment.body, '@donpetry-bot') &&
-       !contains(github.event.comment.body, 'pr-review-agent mention-ack') &&
+       github.event.comment.user.login != 'donpetry-bot' &&
+       !contains(github.event.comment.body, '<!-- pr-review-agent') &&
        contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association) &&
        (github.event_name == 'pull_request_review_comment' ||
         github.event.issue.pull_request != null))
@@ -143,14 +152,32 @@ jobs:
         env:
           GH_TOKEN: ${{ secrets.DON_PETRY_BOT_GH_PAT }}
           PR_URL: ${{ steps.pr.outputs.pr_url }}
+          # Pass event fields via env, never interpolated inline into the shell
+          # body — a login is attacker-controlled and inline `${{ }}` in a run:
+          # block is a script-injection vector.
+          EVENT_NAME: ${{ github.event_name }}
+          SENDER_LOGIN: ${{ github.event.sender.login }}
+          COMMENT_USER: ${{ github.event.comment.user.login }}
+          BOT_LOGIN: donpetry-bot
         run: |
-          if [ "${{ github.event_name }}" = "pull_request" ]; then
-            ACTOR="${{ github.event.sender.login }}"
-            MSG="@${ACTOR} assigned me as reviewer — starting a fresh review now. Results will appear in a few minutes."
+          if [ "$EVENT_NAME" = "pull_request" ]; then
+            ACTOR="$SENDER_LOGIN"
+            ACTION="assigned me as reviewer — starting a fresh review now. Results will appear in a few minutes."
           else
-            ACTOR="${{ github.event.comment.user.login }}"
-            MSG="@${ACTOR} I'm on it — starting a fresh review now. Results will appear in a few minutes."
+            ACTOR="$COMMENT_USER"
+            ACTION="I'm on it — starting a fresh review now. Results will appear in a few minutes."
+          fi
+          # Never emit a literal '@donpetry-bot' (#538): an @-mention of the bot
+          # is itself an issue_comment the mention listener reads as a review
+          # request, self-looping the cascade. When the actor IS the bot, address
+          # it by plain login (no leading '@' → fires no mention webhook); a human
+          # requester still gets a real @-mention ping.
+          if [ "$ACTOR" = "$BOT_LOGIN" ]; then
+            ADDRESSEE="$ACTOR"
+          else
+            ADDRESSEE="@${ACTOR}"
           fi
+          MSG="${ADDRESSEE} ${ACTION}"
           # Best-effort (issue #594): the ack comment ran unguarded under `bash -e`,
           # so a single PR at GitHub's 2500-comment cap ("Commenting is disabled on
           # issues with more than 2500 comments"), a secondary rate limit, or a

.github/workflows/pr-review-mention-tests.yml

@@ -0,0 +1,62 @@
+# Quality gates for the reusable pr-review-mention workflow.
+#
+# Triggered on any PR that touches:
+#   - .github/workflows/pr-review-mention*.yml
+#   - test/workflows/pr-review-mention/**
+#   - standards/workflows/pr-review-mention.yml
+#
+# Gates (all must pass before merge):
+#   1. shellcheck — static analysis for the test gh stub
+#   2. bats       — guard (self-trigger exclusion) + ack (no bot self-mention)
+#
+# Standard: pins issue #538 — the mention listener must never re-trigger itself.
+
+name: PR Review Mention Tests
+
+on:
+  pull_request:
+    paths:
+      - '.github/workflows/pr-review-mention*.yml'
+      - 'test/workflows/pr-review-mention/**'
+      - 'standards/workflows/pr-review-mention.yml'
+  push:
+    branches:
+      - main
+    paths:
+      - '.github/workflows/pr-review-mention*.yml'
+      - 'test/workflows/pr-review-mention/**'
+      - 'standards/workflows/pr-review-mention.yml'
+
+permissions:
+  contents: read
+
+concurrency:
+  group: pr-review-mention-tests-${{ github.ref }}
+  cancel-in-progress: true
+
+jobs:
+  test:
+    name: Lint and bats
+    runs-on: ubuntu-latest
+    timeout-minutes: 10
+    steps:
+      - name: Checkout repository
+        uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
+        with:
+          fetch-depth: 1
+
+      - name: Install bats, shellcheck, and jq
+        # yq (mikefarah) is preinstalled on the ubuntu-latest runner image and is
+        # what helpers/setup.bash uses to read the workflow under test.
+        run: |
+          set -euo pipefail
+          sudo apt-get update -qq
+          sudo apt-get install -y --no-install-recommends bats shellcheck jq
+
+      - name: shellcheck
+        run: |
+          set -euo pipefail
+          shellcheck -x test/workflows/pr-review-mention/stubs/gh
+
+      - name: Run bats suite
+        run: bats --print-output-on-failure test/workflows/pr-review-mention/

test/workflows/pr-review-mention/ack.bats

@@ -0,0 +1,76 @@
+#!/usr/bin/env bats
+# Tests for the "Post acknowledgement comment" step in
+# .github/workflows/pr-review-mention-reusable.yml.
+#
+# Pins issue #538 acceptance: "The ack comment fires no mention webhook." A
+# literal `@donpetry-bot` in the ack is itself an issue_comment event that the
+# mention listener reads as a review request → self-loop. So when the actor
+# being acknowledged IS the bot, the ack must address it by plain login (no
+# leading `@`, which fires no mention webhook); a human requester still gets a
+# real `@`-mention. The ack always carries the agent marker for defense in depth.
+#
+# The step's `run:` script is extracted from the workflow and executed for real
+# against a fake `gh` that records the exact `--body`, so the assertions exercise
+# the shipped logic — not a reimplementation.
+
+load 'helpers/setup'
+
+setup() {
+  tt_make_tmpdir
+  tt_install_gh_stub
+  GH_BODY_FILE="${TT_TMP}/body.txt"
+  export GH_BODY_FILE
+  ACK_SCRIPT="${TT_TMP}/ack.sh"
+  tt_step_run "Post acknowledgement comment" >"$ACK_SCRIPT"
+}
+
+teardown() {
+  tt_cleanup_tmpdir
+}
+
+# Run the extracted ack step with the env a GitHub run: block would receive.
+# Any env the caller pre-exports (EVENT_NAME, SENDER_LOGIN, COMMENT_USER) is
+# honoured; the rest get harmless defaults.
+run_ack() {
+  GH_TOKEN="x" \
+  PR_URL="https://github.com/petry-projects/.github/pull/1" \
+  BOT_LOGIN="donpetry-bot" \
+  EVENT_NAME="${EVENT_NAME:-issue_comment}" \
+  SENDER_LOGIN="${SENDER_LOGIN:-someone}" \
+  COMMENT_USER="${COMMENT_USER:-someone}" \
+    bash "$ACK_SCRIPT"
+}
+
+body() { cat "$GH_BODY_FILE"; }
+
+# ── human requester: real @-mention, marker present ──────────────────────────
+
+@test "ack: a human commenter is @-mentioned in the acknowledgement" {
+  EVENT_NAME="issue_comment" COMMENT_USER="alice" run_ack
+  [ -f "$GH_BODY_FILE" ]
+  [[ "$(body)" == *"@alice"* ]]
+}
+
+@test "ack: the acknowledgement always carries the pr-review-agent marker" {
+  EVENT_NAME="issue_comment" COMMENT_USER="alice" run_ack
+  [[ "$(body)" == *"<!-- pr-review-agent mention-ack -->"* ]]
+}
+
+# ── the #538 self-mention kill ───────────────────────────────────────────────
+
+@test "ack: never emits a literal @donpetry-bot when the bot is the actor (comment path)" {
+  EVENT_NAME="issue_comment" COMMENT_USER="donpetry-bot" run_ack
+  [[ "$(body)" != *"@donpetry-bot"* ]]
+  # still addresses the bot, just without the mention-firing '@'
+  [[ "$(body)" == *"donpetry-bot"* ]]
+}
+
+@test "ack: never emits a literal @donpetry-bot when the bot self-assigns as reviewer (pull_request path)" {
+  EVENT_NAME="pull_request" SENDER_LOGIN="donpetry-bot" run_ack
+  [[ "$(body)" != *"@donpetry-bot"* ]]
+}
+
+@test "ack: a human reviewer-assigner is still @-mentioned (pull_request path)" {
+  EVENT_NAME="pull_request" SENDER_LOGIN="bob" run_ack
+  [[ "$(body)" == *"@bob"* ]]
+}

test/workflows/pr-review-mention/guard.bats

@@ -0,0 +1,57 @@
+#!/usr/bin/env bats
+# Tests for the handle-mention job-level `if:` guard in
+# .github/workflows/pr-review-mention-reusable.yml.
+#
+# Pins issue #538: the mention listener must never re-trigger itself. The
+# acknowledgement comment the cascade posts @-mentions donpetry-bot and carries
+# an `<!-- pr-review-agent … -->` marker; without an exclusion that ack is an
+# `issue_comment: created` event the guard reads as a fresh review request, so
+# the cascade self-loops (.github-private#860: 1,481 identical acks over ~4.5h).
+#
+# The guard MUST stay a job-level `if:` (not a runtime step): it gates whether
+# the job — and its secrets — start at all, so a bot/agent comment must be
+# rejected before the job spins up. These tests therefore assert on the guard
+# expression itself rather than a shell reimplementation.
+
+load 'helpers/setup'
+
+# ── recursion kill: bot-authored + agent-marker comments must be excluded ─────
+
+@test "guard: excludes comments authored by donpetry-bot itself" {
+  guard="$(tt_job_if)"
+  [[ "$guard" == *"github.event.comment.user.login != 'donpetry-bot'"* ]]
+}
+
+@test "guard: excludes any comment carrying an <!-- pr-review-agent marker" {
+  # Broadened from the exact 'pr-review-agent mention-ack' string so EVERY agent
+  # marker (review results, mention-ack, future markers) is caught, not just one.
+  guard="$(tt_job_if)"
+  [[ "$guard" == *"!contains(github.event.comment.body, '<!-- pr-review-agent')"* ]]
+}
+
+@test "guard: no longer keys the exclusion on the narrow mention-ack string alone" {
+  # The pre-#538 guard only skipped 'pr-review-agent mention-ack'; any other
+  # agent-marked comment slipped through. Assert the narrow-only form is gone.
+  guard="$(tt_job_if)"
+  [[ "$guard" != *"'pr-review-agent mention-ack'"* ]]
+}
+
+# ── the human trigger path must still work ───────────────────────────────────
+
+@test "guard: still fires on an @donpetry-bot mention" {
+  guard="$(tt_job_if)"
+  [[ "$guard" == *"contains(github.event.comment.body, '@donpetry-bot')"* ]]
+}
+
+@test "guard: still gates comment events on trusted author_association" {
+  guard="$(tt_job_if)"
+  [[ "$guard" == *'OWNER'* ]]
+  [[ "$guard" == *'MEMBER'* ]]
+  [[ "$guard" == *'COLLABORATOR'* ]]
+  [[ "$guard" == *'github.event.comment.author_association'* ]]
+}
+
+@test "guard: still fires when donpetry-bot is assigned as reviewer" {
+  guard="$(tt_job_if)"
+  [[ "$guard" == *"github.event.requested_reviewer.login == 'donpetry-bot'"* ]]
+}

test/workflows/pr-review-mention/helpers/setup.bash

@@ -0,0 +1,49 @@
+#!/usr/bin/env bash
+# Common test helpers for the pr-review-mention bats suite.
+# Mirrors the pattern in test/workflows/feature-ideation/helpers/setup.bash.
+
+# Repo root, regardless of where bats is invoked from.
+TT_REPO_ROOT="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")/../../../.." && pwd)"
+export TT_REPO_ROOT
+
+TT_WORKFLOW="${TT_REPO_ROOT}/.github/workflows/pr-review-mention-reusable.yml"
+export TT_WORKFLOW
+
+TT_STUBS_DIR="${TT_REPO_ROOT}/test/workflows/pr-review-mention/stubs"
+export TT_STUBS_DIR
+
+# Per-test scratch dir, auto-cleaned by tt_cleanup_tmpdir in teardown.
+tt_make_tmpdir() {
+  TT_TMP="$(mktemp -d)"
+  export TT_TMP
+}
+
+tt_cleanup_tmpdir() {
+  if [ -n "${TT_TMP:-}" ] && [ -d "${TT_TMP}" ]; then
+    rm -rf "${TT_TMP}"
+  fi
+}
+
+# Install the fake `gh` binary on PATH for the duration of a test.
+# Behavior (body capture) is driven by GH_BODY_FILE — see stubs/gh.
+tt_install_gh_stub() {
+  local stub_dir="${TT_TMP}/bin"
+  mkdir -p "$stub_dir"
+  cp "${TT_STUBS_DIR}/gh" "$stub_dir/gh"
+  chmod +x "$stub_dir/gh"
+  PATH="${stub_dir}:${PATH}"
+  export PATH
+}
+
+# Extract the job-level `if:` guard of the handle-mention job, with runs of
+# whitespace collapsed to single spaces so structural assertions are immune to
+# the guard's multi-line YAML formatting.
+tt_job_if() {
+  yq -r '.jobs.handle-mention.if' "$TT_WORKFLOW" | tr '\n' ' ' | tr -s ' '
+}
+
+# Extract the `run:` script of a named step in the handle-mention job.
+tt_step_run() {
+  local step_name="$1"
+  yq -r ".jobs.handle-mention.steps[] | select(.name == \"${step_name}\") | .run" "$TT_WORKFLOW"
+}

test/workflows/pr-review-mention/stubs/gh

@@ -0,0 +1,32 @@
+#!/usr/bin/env bash
+# Fake `gh` binary for the pr-review-mention ack tests.
+#
+# Its only job is to capture the value passed to `--body` verbatim so a test can
+# assert on the exact acknowledgement text the workflow would post — in
+# particular that it never contains a literal `@donpetry-bot` self-mention.
+#
+# Recognized vars:
+#   GH_BODY_FILE  — file to write the `--body` value to (verbatim, no quoting)
+#   GH_STUB_LOG   — optional append-only argv log (shell-quoted via %q)
+#   GH_STUB_EXIT  — exit code (default 0)
+
+set -euo pipefail
+
+if [ -n "${GH_STUB_LOG:-}" ]; then
+  # shellcheck disable=SC2059
+  printf '%q ' "$@" >>"$GH_STUB_LOG"
+  printf '\n' >>"$GH_STUB_LOG"
+fi
+
+if [ -n "${GH_BODY_FILE:-}" ]; then
+  prev=""
+  for arg in "$@"; do
+    if [ "$prev" = "--body" ]; then
+      printf '%s' "$arg" >"$GH_BODY_FILE"
+      break
+    fi
+    prev="$arg"
+  done
+fi
+
+exit "${GH_STUB_EXIT:-0}"