diff --git a/.github/scripts/pr-review-mention/lib/force-review.sh b/.github/scripts/pr-review-mention/lib/force-review.sh new file mode 100644 index 000000000..65a4c8af3 --- /dev/null +++ b/.github/scripts/pr-review-mention/lib/force-review.sh @@ -0,0 +1,73 @@ +#!/usr/bin/env bash +# force_review decision for the pr-review-mention reusable workflow (#1281). +# +# Pure and side-effect-free, so it can be unit-tested with bats (see +# test/workflows/pr-review-mention/force-review.bats). The reusable sources this +# file from its own commit and acts on the verdict in "Trigger review agent". +# +# pr-review.yml in .github-private sets FORCE_REVIEW (skip the advisory-bot and +# comment-disposition waiting gates; the review itself still runs in full) only +# when the dispatch carries client_payload.force_review == true. The rule: +# - a trusted human's @donpetry-bot comment (OWNER / MEMBER / COLLABORATOR, +# not donpetry-bot, no automation marker) is the break-glass → force; +# - the review_requested path never forces; +# - a comment carrying an automation marker never forces. Agents post as the +# owner account, so the association alone cannot tell them from a person. +# Known, accepted limit: an agent posting as the owner WITHOUT a marker is +# indistinguishable from the maintainer and is forced too. + +# Body substrings that identify an agent-posted comment. +PR_REVIEW_MENTION_AUTOMATION_MARKERS=( + '\n%s' "${MSG}")" \ || echo "::warning::could not post ack comment on $PR_URL (comment cap / API error) — continuing" + # Source the force_review decision from this reusable's own commit so the + # rule always matches the pinned workflow version. Authenticated with the + # workflows PAT (already required here) so callers need no extra + # `contents:` permission. + - name: Checkout force-review tooling + if: steps.trust.outputs.trusted == 'true' + # Best-effort: a checkout failure must not suppress the review dispatch; + # the trigger step falls back to a plain (non-forced) dispatch. + continue-on-error: true + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + repository: petry-projects/.github + ref: ${{ github.job_workflow_sha }} + path: .pr-review-mention-tooling + token: ${{ secrets.GH_PAT_DON_PETRY || secrets.GH_PAT_WORKFLOWS }} + fetch-depth: 1 + persist-credentials: false + - name: Trigger review agent if: steps.trust.outputs.trusted == 'true' env: GH_TOKEN: ${{ secrets.GH_PAT_DON_PETRY || secrets.GH_PAT_WORKFLOWS }} PR_URL: ${{ steps.pr.outputs.pr_url }} + # Comment fields via env, never inline `${{ }}` (script-injection). + EVENT_NAME: ${{ github.event_name }} + COMMENT_ASSOC: ${{ github.event.comment.author_association }} + COMMENT_USER: ${{ github.event.comment.user.login }} + COMMENT_BODY: ${{ github.event.comment.body }} run: | + LIB=.pr-review-mention-tooling/.github/scripts/pr-review-mention/lib/force-review.sh + if [ -f "$LIB" ]; then + # shellcheck source=/dev/null + . "$LIB" + # Pure decision (see the header comment): force | plain | none. + read -r VERDICT RULE < <(pr_review_mention_force_decision \ + "$EVENT_NAME" "$COMMENT_ASSOC" "$COMMENT_USER" "$COMMENT_BODY") + else + echo "::warning::force-review tooling unavailable — dispatching a plain review" + VERDICT=plain + RULE=tooling-unavailable + fi + + FORCE_ARGS=() + case "$VERDICT" in + force) + # -F (typed field) so the payload value is the JSON boolean true. + FORCE_ARGS=(-F "client_payload[force_review]=true") + echo "force_review=true (rule: $RULE)" + ;; + plain) + echo "force_review=false (rule: $RULE)" + ;; + *) + echo "No dispatch (rule: $RULE)" + exit 0 + ;; + esac + # repository_dispatch requires only Contents: write, not Actions: write. # pr-review.yml in .github-private listens for "pr-review-mention" and - # extracts client_payload.pr_url to run the cascade. + # extracts client_payload.pr_url (and force_review) to run the cascade. gh api \ --method POST \ --header "Accept: application/vnd.github+json" \ /repos/petry-projects/.github-private/dispatches \ --field event_type=pr-review-mention \ - --field "client_payload[pr_url]=$PR_URL" + --field "client_payload[pr_url]=$PR_URL" \ + "${FORCE_ARGS[@]}" echo "Dispatch sent for $PR_URL" diff --git a/.github/workflows/pr-review-mention-tests.yml b/.github/workflows/pr-review-mention-tests.yml index 82cfe7c5d..d05971871 100644 --- a/.github/workflows/pr-review-mention-tests.yml +++ b/.github/workflows/pr-review-mention-tests.yml @@ -4,10 +4,12 @@ # - .github/workflows/pr-review-mention*.yml # - test/workflows/pr-review-mention/** # - standards/workflows/pr-review-mention.yml +# - .github/scripts/pr-review-mention/** # # Gates (all must pass before merge): -# 1. shellcheck — static analysis for the test gh stub +# 1. shellcheck — static analysis for the test gh stub and force-review lib # 2. bats — guard (self-trigger exclusion) + ack (no bot self-mention) +# + force-review (break-glass force_review decision, #1281) # # Standard: pins issue #538 — the mention listener must never re-trigger itself. @@ -19,6 +21,7 @@ on: - '.github/workflows/pr-review-mention*.yml' - 'test/workflows/pr-review-mention/**' - 'standards/workflows/pr-review-mention.yml' + - '.github/scripts/pr-review-mention/**' push: branches: - main @@ -26,6 +29,7 @@ on: - '.github/workflows/pr-review-mention*.yml' - 'test/workflows/pr-review-mention/**' - 'standards/workflows/pr-review-mention.yml' + - '.github/scripts/pr-review-mention/**' permissions: contents: read @@ -80,7 +84,8 @@ jobs: - name: shellcheck run: | set -euo pipefail - shellcheck -x test/workflows/pr-review-mention/stubs/gh + shellcheck -x test/workflows/pr-review-mention/stubs/gh \ + .github/scripts/pr-review-mention/lib/force-review.sh - name: Run bats suite run: bats --print-output-on-failure test/workflows/pr-review-mention/ diff --git a/test/workflows/pr-review-mention/force-review.bats b/test/workflows/pr-review-mention/force-review.bats new file mode 100644 index 000000000..173f3b2d8 --- /dev/null +++ b/test/workflows/pr-review-mention/force-review.bats @@ -0,0 +1,244 @@ +#!/usr/bin/env bats +# Tests for the force_review decision in the pr-review-mention listener +# (issue #1281, item 1 of petry-projects/.github-private#2082). +# +# pr-review.yml in .github-private sets FORCE_REVIEW (skip the advisory-bot and +# comment-disposition waiting gates) only when the repository_dispatch carries +# client_payload.force_review == true. A trusted human's `@donpetry-bot` comment +# is the documented break-glass, so it must send the flag. Nothing else may: +# the review_requested path never does, and a comment carrying an automation +# marker never does either. Agents post as the owner account, so the author +# association alone cannot tell them from a person. +# +# Two layers are covered: +# 1. the pure decision function in +# .github/scripts/pr-review-mention/lib/force-review.sh, over the whole matrix; +# 2. the shipped "Trigger review agent" step, extracted from the workflow and +# run against the fake `gh`. The assertions are on the exact dispatch argv. + +load 'helpers/setup' + +LIB="${TT_REPO_ROOT}/.github/scripts/pr-review-mention/lib/force-review.sh" + +setup() { + tt_make_tmpdir + GH_STUB_LOG="${TT_TMP}/gh.log" + export GH_STUB_LOG + : >"$GH_STUB_LOG" + # shellcheck source=/dev/null + . "$LIB" +} + +teardown() { + tt_cleanup_tmpdir +} + +decide() { pr_review_mention_force_decision "$@"; } + +MARKERS=( + '\n@donpetry-bot please review' + [ "$status" -eq 0 ] + [ "$output" = "plain automation-marker" ] +} + +@test "decision: every automation marker → plain" { + local m + for m in "${MARKERS[@]}"; do + run decide issue_comment OWNER don-petry "${m} x -->"$'\n''@donpetry-bot review' + [ "$status" -eq 0 ] + [ "$output" = "plain automation-marker" ] || { + echo "marker not honoured: $m → $output" + return 1 + } + done +} + +@test "decision: a marker anywhere in the body (not just the start) → plain" { + run decide issue_comment OWNER don-petry $'@donpetry-bot please review\n\n' + [ "$status" -eq 0 ] + [ "$output" = "plain automation-marker" ] +} + +@test "decision: CONTRIBUTOR comment → none (no dispatch)" { + run decide issue_comment CONTRIBUTOR mallory '@donpetry-bot please review' + [ "$status" -eq 0 ] + [ "$output" = "none untrusted-association" ] +} + +@test "decision: NONE / FIRST_TIME_CONTRIBUTOR / empty association → none" { + local a + for a in NONE FIRST_TIME_CONTRIBUTOR FIRST_TIMER MANNEQUIN ""; do + run decide issue_comment "$a" mallory '@donpetry-bot please review' + [ "$status" -eq 0 ] + [ "$output" = "none untrusted-association" ] + done +} + +@test "decision: comment by donpetry-bot itself → none" { + run decide issue_comment OWNER donpetry-bot '@donpetry-bot please review' + [ "$status" -eq 0 ] + [ "$output" = "none bot-author" ] +} + +@test "decision: unknown event → none" { + run decide workflow_dispatch OWNER don-petry '@donpetry-bot' + [ "$status" -eq 0 ] + [ "$output" = "none unsupported-event" ] +} + +@test "decision: a body full of glob/shell metacharacters is treated literally" { + run decide issue_comment OWNER don-petry "@donpetry-bot \$(touch ${TT_TMP}/pwned) * ? [a] \`id\`" + [ "$status" -eq 0 ] + [ "$output" = "force trusted-human-comment-mention" ] + [ ! -e "${TT_TMP}/pwned" ] +} + +# ── shipped "Trigger review agent" step ────────────────────────────────────── + +# Run the extracted step from a scratch cwd where the tooling checkout path +# resolves to this repo, with the env a GitHub run: block would receive. +run_trigger() { + tt_install_gh_stub + tt_step_run "Trigger review agent" >"${TT_TMP}/trigger.sh" + ln -s "$TT_REPO_ROOT" "${TT_TMP}/.pr-review-mention-tooling" + ( + cd "$TT_TMP" + GH_TOKEN="x" \ + PR_URL="https://github.com/petry-projects/.github/pull/1" \ + EVENT_NAME="${EVENT_NAME:-issue_comment}" \ + COMMENT_ASSOC="${COMMENT_ASSOC:-}" \ + COMMENT_USER="${COMMENT_USER:-}" \ + COMMENT_BODY="${COMMENT_BODY:-}" \ + bash -e "${TT_TMP}/trigger.sh" + ) +} + +gh_log() { cat "$GH_STUB_LOG"; } + +@test "step: OWNER comment mention dispatches force_review=true as a JSON boolean (-F)" { + EVENT_NAME=issue_comment COMMENT_ASSOC=OWNER COMMENT_USER=don-petry \ + COMMENT_BODY='@donpetry-bot please review' run run_trigger + [ "$status" -eq 0 ] + [[ "$(gh_log)" == *"-F client_payload\[force_review\]=true"* ]] + [[ "$(gh_log)" == *"client_payload\[pr_url\]="* ]] + [[ "$output" == *"force_review=true (rule: trusted-human-comment-mention)"* ]] +} + +@test "step: the flag is never sent as a raw string (-f / --raw-field)" { + EVENT_NAME=issue_comment COMMENT_ASSOC=OWNER COMMENT_USER=don-petry \ + COMMENT_BODY='@donpetry-bot please review' run run_trigger + [[ "$(gh_log)" != *"-f client_payload\[force_review\]"* ]] + [[ "$(gh_log)" != *"--raw-field client_payload\[force_review\]"* ]] +} + +@test "step: review_requested dispatches without force_review" { + EVENT_NAME=pull_request COMMENT_ASSOC="" COMMENT_USER="" COMMENT_BODY="" run run_trigger + [ "$status" -eq 0 ] + [[ "$(gh_log)" == *"/repos/petry-projects/.github-private/dispatches"* ]] + [[ "$(gh_log)" != *"force_review"* ]] + [[ "$output" == *"force_review=false (rule: review-requested)"* ]] +} + +@test "step: OWNER comment with a \n@donpetry-bot please review' run run_trigger + [ "$status" -eq 0 ] + [[ "$(gh_log)" == *"/repos/petry-projects/.github-private/dispatches"* ]] + [[ "$(gh_log)" != *"force_review"* ]] + [[ "$output" == *"force_review=false (rule: automation-marker)"* ]] +} + +@test "step: CONTRIBUTOR comment sends no dispatch at all" { + EVENT_NAME=issue_comment COMMENT_ASSOC=CONTRIBUTOR COMMENT_USER=mallory \ + COMMENT_BODY='@donpetry-bot please review' run run_trigger + [ "$status" -eq 0 ] + [ -z "$(gh_log)" ] + [[ "$output" == *"untrusted-association"* ]] +} + +@test "step: logs exactly one force_review decision line" { + EVENT_NAME=issue_comment COMMENT_ASSOC=OWNER COMMENT_USER=don-petry \ + COMMENT_BODY='@donpetry-bot please review' run run_trigger + [ "$(grep -c 'force_review=' <<<"$output")" -eq 1 ] +} + +# ── wiring ─────────────────────────────────────────────────────────────────── + +@test "wiring: tooling is checked out from this reusable's own commit" { + run yq -r '.jobs.handle-mention.steps[] | select(.name == "Checkout force-review tooling") | .with.ref' "$TT_WORKFLOW" + [ "$status" -eq 0 ] + [ "$output" = '${{ github.job_workflow_sha }}' ] + run yq -r '.jobs.handle-mention.steps[] | select(.name == "Checkout force-review tooling") | .with.path' "$TT_WORKFLOW" + [ "$status" -eq 0 ] + [ "$output" = '.pr-review-mention-tooling' ] +} + +@test "wiring: comment body reaches the trigger step via env, never inline \${{ }}" { + run yq -r '.jobs.handle-mention.steps[] | select(.name == "Trigger review agent") | .env.COMMENT_BODY' "$TT_WORKFLOW" + [ "$status" -eq 0 ] + [ "$output" = '${{ github.event.comment.body }}' ] + run tt_step_run "Trigger review agent" + [ "$status" -eq 0 ] + [[ "$output" != *'${{'* ]] +} + +@test "wiring: the workflow header comment states the force_review rule" { + head -40 "$TT_WORKFLOW" | grep -q 'force_review' +} + +@test "decision: a [bot] account with a trusted association → none" { + run decide issue_comment COLLABORATOR 'some-app[bot]' '@donpetry-bot please review' + [ "$status" -eq 0 ] + [ "$output" = "none bot-author" ] +} + +@test "wiring: tooling checkout is best-effort so a failure cannot suppress the dispatch" { + run yq -r '.jobs.handle-mention.steps[] | select(.name == "Checkout force-review tooling") | .["continue-on-error"]' "$TT_WORKFLOW" + [ "$status" -eq 0 ] + [ "$output" = "true" ] +}