Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
don-petry wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions .github/workflows/pr-review-mention-reusable.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,20 @@ jobs:
# team review requests; comment fields don't exist on pull_request events.
# 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.
#
# Self-trigger guard (#538): a comment authored by donpetry-bot, or one
# carrying a `<!-- pr-review-agent ... -->` marker, is the agent talking
# about its own work and must never re-fire the listener. Without this the
# mention-ack (@-mention authored by the bot, marker-tagged) re-triggers the
# cascade — the #860 runaway (1,481 identical acks at ~9s for ~4.5h).
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' &&
github.event.comment.user.login != 'donpetry-bot' &&
!contains(github.event.comment.body, '<!-- pr-review-agent') &&
contains(github.event.comment.body, '@donpetry-bot') &&
contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association) &&
(github.event_name == 'pull_request_review_comment' ||
Expand Down Expand Up @@ -134,12 +142,16 @@ jobs:
GH_TOKEN: ${{ secrets.DON_PETRY_BOT_GH_PAT }}
PR_URL: ${{ steps.pr.outputs.pr_url }}
run: |
# Reference the actor in PLAIN TEXT (no leading @): a literal @-mention
# fires a mention webhook, and an ack that ever named the bot would
# re-trigger this listener. Defense-in-depth alongside the job `if:`
# self-trigger guard above (#538 / #860).
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."
MSG="${ACTOR} 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."
MSG="${ACTOR} I'm on it — starting a fresh review now. Results will appear in a few minutes."
fi
gh pr comment "$PR_URL" --body "$(printf '<!-- pr-review-agent mention-ack -->\n%s' "${MSG}")"

Expand Down
55 changes: 55 additions & 0 deletions .github/workflows/pr-review-mention-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
# Quality gate 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
#
# Gate (must pass before merge):
# 1. bats — contract tests for the self-trigger guard and ack text (#538)
#
# Standard: this workflow pins the anti-self-trigger contract from issue #538
# (the org-repo half of the #860 mention-ack runaway).

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: 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
run: |
set -euo pipefail
sudo apt-get update -qq
sudo apt-get install -y --no-install-recommends bats

- name: Run bats suite
run: bats --print-output-on-failure test/workflows/pr-review-mention/
29 changes: 29 additions & 0 deletions test/workflows/pr-review-mention/helpers/setup.bash
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# Common test helpers for the pr-review-mention bats suite.

# Repo root, regardless of where bats is invoked from.
TT_REPO_ROOT="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")/../../../.." && pwd)"
export TT_REPO_ROOT

# The reusable workflow that owns all mention-dispatch logic (the file under test).
TT_REUSABLE="${TT_REPO_ROOT}/.github/workflows/pr-review-mention-reusable.yml"
export TT_REUSABLE

# Print the job-level `if:` guard block: everything from the `if: |` line up to
# (but not including) the `steps:` key of the handle-mention job.
prm_if_guard() {
awk '
/^[[:space:]]*if:[[:space:]]*\|/ { grab=1; next }
grab && /^[[:space:]]*steps:[[:space:]]*$/ { exit }
grab { print }
' "$TT_REUSABLE"
Comment on lines +14 to +18

@gemini-code-assist gemini-code-assist Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The current awk parser is tightly coupled to the block-scalar syntax if: |. If the workflow is ever refactored to use a single-line if: statement or a different block-scalar indicator like >, this helper will fail to extract the guard condition.

We can make the parser more robust by:

  1. Supporting both | and > block-scalar indicators.
  2. Supporting single-line if: statements by stripping the if: prefix and printing the condition directly.
  awk '
    /^[[:space:]]*if:[[:space:]]*[|>]/ { grab=1; next }
    /^[[:space:]]*if:[[:space:]]+/ { sub(/^[[:space:]]*if:[[:space:]]*/, ""); print; next }
    grab && /^[[:space:]]*steps:[[:space:]]*$/ { exit }
    grab { print }
  ' "$TT_REUSABLE"

}

# Print the body of the "Post acknowledgement comment" step: everything from that
# step's `- name:` line up to (but not including) the next `- name:` line.
prm_ack_step() {
awk '
/^[[:space:]]*-[[:space:]]*name:[[:space:]]*Post acknowledgement comment/ { grab=1; print; next }
grab && /^[[:space:]]*-[[:space:]]*name:/ { exit }
grab { print }
' "$TT_REUSABLE"
}
68 changes: 68 additions & 0 deletions test/workflows/pr-review-mention/self-trigger.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
#!/usr/bin/env bats
# Contract tests for .github/workflows/pr-review-mention-reusable.yml
#
# Pins issue #538: the mention listener must never trigger on its own work.
# The pr-review ack comment is authored by donpetry-bot and carries a
# `<!-- pr-review-agent ... -->` marker; either property must keep the job's
# `if:` gate (and its secrets) from ever starting. The ack must also fire no
# mention webhook (#860 runaway: 1,481 byte-identical acks at ~9s cadence).

load 'helpers/setup'

@test "reusable workflow file exists" {
[ -f "$TT_REUSABLE" ]
}

# ── Acceptance 1: bot-authored / marker comments never re-trigger ─────────────

@test "if-guard excludes comments authored by the bot itself" {
run prm_if_guard
[ "$status" -eq 0 ]
[[ "$output" == *"github.event.comment.user.login != 'donpetry-bot'"* ]]

@gemini-code-assist gemini-code-assist Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The assertion uses strict string matching with single quotes ('donpetry-bot'). If the workflow file is modified to use double quotes (e.g., "donpetry-bot") or if whitespace around != is adjusted, this test will fail.

Using a regular expression match (=~) makes the test much more resilient to minor formatting changes.

  [[ "$output" =~ github\.event\.comment\.user\.login[[:space:]]*!=[[:space:]]*['"]donpetry-bot['"] ]]

}

@test "if-guard excludes comments carrying a pr-review-agent marker" {
run prm_if_guard
[ "$status" -eq 0 ]
[[ "$output" == *"!contains(github.event.comment.body, '<!-- pr-review-agent')"* ]]

@gemini-code-assist gemini-code-assist Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Similar to the previous test, using strict string matching with single quotes makes the test fragile to quote style changes or whitespace adjustments. Using a regular expression match (=~) is more robust.

  [[ "$output" =~ !contains\(github\.event\.comment\.body,[[:space:]]*['"]<!--[[:space:]]*pr-review-agent['"]\) ]]

}

@test "if-guard still requires an @donpetry-bot mention to fire (trigger preserved)" {
run prm_if_guard
[ "$status" -eq 0 ]
[[ "$output" == *"contains(github.event.comment.body, '@donpetry-bot')"* ]]

@gemini-code-assist gemini-code-assist Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using a regular expression match (=~) instead of strict string matching prevents the test from breaking if the quote style or whitespace changes in the workflow file.

  [[ "$output" =~ contains\(github\.event\.comment\.body,[[:space:]]*['"]@donpetry-bot['"]\) ]]

}

@test "if-guard still gates on trusted author_association (trust gate preserved)" {
run prm_if_guard
[ "$status" -eq 0 ]
[[ "$output" == *'["OWNER","MEMBER","COLLABORATOR"]'* ]]

@gemini-code-assist gemini-code-assist Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The strict string match for the JSON array ["OWNER","MEMBER","COLLABORATOR"] is fragile to formatting changes (such as adding spaces after commas or switching to single quotes). A regular expression match is more resilient.

  [[ "$output" =~ \\[[[:space:]]*['"]OWNER['"][[:space:]]*,[[:space:]]*['"]MEMBER['"][[:space:]]*,[[:space:]]*['"]COLLABORATOR['"][[:space:]]*\\] ]]

}

# ── Acceptance 2: the ack comment fires no mention webhook ────────────────────

@test "ack step never @-mentions any user (no mention webhook)" {
run prm_ack_step
[ "$status" -eq 0 ]
# A mention webhook fires only on a literal `@handle`. The ack must reference
# the actor as plain text, so no `@$` / `@${` interpolation may remain.
[[ "$output" != *'@$'* ]]
}

@test "ack step never literally @-mentions donpetry-bot" {
run prm_ack_step
[ "$status" -eq 0 ]
[[ "$output" != *"@donpetry-bot"* ]]
}

@test "ack step still tags its comment with the pr-review-agent marker" {
run prm_ack_step
[ "$status" -eq 0 ]
[[ "$output" == *"<!-- pr-review-agent mention-ack -->"* ]]
}

@test "ack step still names the actor in the acknowledgement (plain text)" {
run prm_ack_step
[ "$status" -eq 0 ]
[[ "$output" == *'${ACTOR}'* ]]
}
Loading