Skip to content
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
9011e68
feat: implement issue #508 — Compliance: stub-surface-drift-pr-auto-r…
don-petry Sep 25, 2026
ece8b1a
chore: dev-lead update (review-changes) [skip ci-relay]
don-petry Sep 25, 2026
0d940e6
Merge branch 'main' into dev-lead/issue-508-20260925-1750
don-petry Oct 2, 2026
fe7fdfc
Merge branch 'main' into dev-lead/issue-508-20260925-1750
don-petry Oct 2, 2026
3ae4e26
fix(bot): improve docstring coverage in test-pr-auto-review-workflow.…
don-petry Oct 10, 2026
cc8137c
fix(bot): improve docstring coverage and validation logic in workflow…
don-petry Oct 10, 2026
07f6b27
fix(reviews): validate full cancel-in-progress expression in pr-auto-…
don-petry Oct 10, 2026
007d440
fix(bot): improve multi-PR detection and race condition prevention in…
don-petry Oct 10, 2026
2397df8
fix(reviews): accept single-PR cancel-in-progress expression in pr-au…
don-petry Oct 10, 2026
4909b5e
fix(bot): add explicit null-safety checks in pr-auto-review concurren…
don-petry Oct 11, 2026
ff1f0ef
fix(bot): restore pr-auto-review concurrency block to match canonical…
don-petry Oct 11, 2026
84556f2
Merge branch 'main' into dev-lead/issue-508-20260925-1750
don-petry Oct 11, 2026
8c99e25
fix(reviews): align pr-auto-review guard with canonical PR-number-onl…
don-petry Oct 11, 2026
8169504
fix(reviews): require PR number in format() args of each concurrency …
don-petry Oct 11, 2026
125a50d
fix(reviews): require PR number as {0} arg and run_id as terminal fal…
don-petry Oct 11, 2026
4285c3a
fix(reviews): reject run-unique values in per-PR concurrency branches
don-petry Oct 11, 2026
a8e8dfe
Merge branch 'main' into dev-lead/issue-508-20260925-1750
don-petry Oct 11, 2026
b653c12
Merge branch 'main' into dev-lead/issue-508-20260925-1750
don-petry Oct 11, 2026
a3bcde4
Merge branch 'main' into dev-lead/issue-508-20260925-1750
don-petry Oct 11, 2026
3bd3e7f
fix(reviews): harden pr-auto-review concurrency guard
don-petry Oct 11, 2026
e851da5
fix(reviews): tighten pr-auto-review concurrency guard checks
don-petry Oct 11, 2026
a5f4cea
fix(reviews): reject escaped fallback placeholder and unwrapped cance…
don-petry Oct 11, 2026
abf1a92
fix(bot): add multi-PR guards to pr-auto-review concurrency conditions
don-petry Oct 11, 2026
594ed0e
fix(bot): resolve concurrency guard logic and race condition in pr-au…
don-petry Oct 11, 2026
7f9a376
fix(reviews): gate pr-auto-review concurrency on pull_requests[0].number
don-petry Oct 11, 2026
e988f39
fix(compliance): restore canonical pr-auto-review concurrency block
don-petry Oct 11, 2026
28d9085
test(compliance): key the event-name mutation on the canonical group …
don-petry Oct 11, 2026
ec9ff01
fix(bot): add multi-PR guard to prevent cross-PR group collisions
don-petry Oct 11, 2026
731c5be
fix(compliance): restore canonical pr-auto-review concurrency block (…
don-petry Oct 11, 2026
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
2 changes: 2 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -122,4 +122,6 @@ jobs:
bash scripts/test-copilot-setup-steps-workflow.sh
bash scripts/test-sonarcloud-workflow.sh
bash scripts/test-sonarcloud-workflow.test.sh
bash scripts/test-pr-auto-review-workflow.sh
Comment thread
don-petry marked this conversation as resolved.
bash scripts/test-pr-auto-review-workflow.test.sh
bash scripts/apply-repo-settings.test.sh
23 changes: 23 additions & 0 deletions .github/workflows/pr-auto-review.yml
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,29 @@ on:

permissions: {}

# Deduplicate ONLY the default-branch-context triggers (issue #1126). On GitHub
# Free (20 concurrent jobs, org-wide) this stub fanned out per trigger. Its runs
# split by context:
# • check_suite / workflow_run — head_branch=main, the check does NOT attach to
# the PR head, so superseding an in-flight run is SAFE. Both events collapse
# onto one group per PR (`…-pr-<number>`) with cancel-in-progress: true.
# • pull_request / pull_request_review — run on the PR head; a cancelled run
# would leave a cancelled `pr-auto-review / check-and-dispatch` check on the
# head. They get a unique-per-run group and never cancel, so they behave
# exactly as before.
# A default-branch trigger with no associated PR (fork / no PR) also falls back to
# the unique-per-run group so unrelated PRs never collapse into one group.
concurrency:
group: >-
${{
(github.event_name == 'check_suite' && github.event.check_suite.pull_requests[0].number)
Comment thread
don-petry marked this conversation as resolved.
&& format('pr-auto-review-ready-check-pr-{0}', github.event.check_suite.pull_requests[0].number)
|| (github.event_name == 'workflow_run' && github.event.workflow_run.pull_requests[0].number)
&& format('pr-auto-review-ready-check-pr-{0}', github.event.workflow_run.pull_requests[0].number)
Comment thread
don-petry marked this conversation as resolved.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment thread
don-petry marked this conversation as resolved.
|| format('pr-auto-review-ready-check-unique-{0}', github.run_id)
}}
Comment thread
don-petry marked this conversation as resolved.
cancel-in-progress: ${{ github.event_name == 'check_suite' || github.event_name == 'workflow_run' }}
Comment thread
don-petry marked this conversation as resolved.
Comment thread
don-petry marked this conversation as resolved.

jobs:
pr-auto-review:
permissions:
Expand Down
160 changes: 160 additions & 0 deletions scripts/test-pr-auto-review-workflow.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
#!/usr/bin/env bash
# Regression guard for pr-auto-review.yml. Locks the event-dependent concurrency
# contract of the thin caller stub (issues #1126, #508) so a future edit cannot
# silently change its deduplication or cancellation behaviour. The stub is synced
# verbatim from petry-projects/.github/standards/workflows/pr-auto-review.yml, so
# this guard enforces the canonical policy:
#
# The stub fans out on check_suite / workflow_run / pull_request(_review). To
# stay under GitHub Free's org-wide concurrency cap it collapses the
# default-branch-context events (check_suite / workflow_run) onto one group per
# PR with cancel-in-progress: true, while pull_request events keep a
# unique-per-run group that never cancels. Invariants:
#
# 1. Per-PR deduplication — each of the check_suite and workflow_run branches
# of the group is keyed on its own event's pull_requests[0].number. Stricter
# forms that also add head_sha and/or a pull_requests[1] guard are accepted.
# 2. No-PR unique fallback — an event with no associated PR falls back to a
# run-unique group (github.run_id) so unrelated work never collapses into
# one cancelable group.
#
# It also asserts cancel-in-progress stays gated on the check_suite / workflow_run
# events (never unconditional), so pull_request-head runs are never cancelled.
#
# Accepts an optional workflow path (default: .github/workflows/pr-auto-review.yml)
# so the checks can be exercised against fixtures — see
# test-pr-auto-review-workflow.test.sh.
# Run: bash scripts/test-pr-auto-review-workflow.sh
set -euo pipefail

WORKFLOW="${1:-.github/workflows/pr-auto-review.yml}"
PASS=true

echo "=== test-pr-auto-review-workflow ==="

# ── Check 0: yq is available ───────────────────────────────────────────────
if ! command -v yq &> /dev/null; then
echo "FAIL: 'yq' is required to parse YAML safely but was not found."
exit 1
fi
echo "PASS: 'yq' is available"

# ── Check 1: file exists ───────────────────────────────────────────────────
if [[ ! -f "$WORKFLOW" ]]; then
echo "FAIL: $WORKFLOW not found"
exit 1
fi
echo "PASS: $WORKFLOW exists"

# ── Check 2: a top-level concurrency.group is present ──────────────────────
group=""
if ! group=$(yq '.concurrency.group' "$WORKFLOW" 2>/dev/null); then
echo "FAIL: yq failed to parse $WORKFLOW. Please check if it is valid YAML."
exit 1
fi
if [[ "$group" == "null" || -z "$group" ]]; then
{
echo "FAIL: no top-level 'concurrency.group' block in $WORKFLOW"
echo " Add a per-PR group so check_suite / workflow_run events"
echo " dedupe per PR and fall back to a run-unique group otherwise."
} >&2
PASS=false
else
echo "PASS: top-level 'concurrency.group' is present in $WORKFLOW"
fi

# The remaining group-shape checks only make sense once a group exists.
if [[ "$group" != "null" && -n "$group" ]]; then
# ── Checks 3-4: per-PR dedup, scoped to each event's own branch ───────────
# Split the (single-line, folded) expression at the workflow_run clause so each
# event is checked against its own branch only; an unbounded match could be
# satisfied by the other event's branch.
wr_marker="github.event_name == 'workflow_run'"
cs_part="${group%%"$wr_marker"*}"
wr_part="${group#*"$wr_marker"}"

if [[ "$group" == *"$wr_marker"* && "$cs_part" =~ check_suite\.pull_requests\[0\]\.number ]]; then
Comment thread
don-petry marked this conversation as resolved.
Outdated
echo "PASS: check_suite group is keyed on the PR number in $WORKFLOW"
else
{
echo "FAIL: check_suite concurrency group is not keyed on the PR number in $WORKFLOW"
echo " Key the check_suite branch on github.event.check_suite.pull_requests[0].number"
} >&2
PASS=false
fi

if [[ "$group" == *"$wr_marker"* && "$wr_part" =~ workflow_run\.pull_requests\[0\]\.number ]]; then
echo "PASS: workflow_run group is keyed on the PR number in $WORKFLOW"
else
{
echo "FAIL: workflow_run concurrency group is not keyed on the PR number in $WORKFLOW"
echo " Key the workflow_run branch on github.event.workflow_run.pull_requests[0].number"
} >&2
PASS=false
fi

# ── Check 5: no-PR unique fallback keyed on github.run_id ──────
if [[ "$group" == *'github.run_id'* ]]; then
Comment thread
don-petry marked this conversation as resolved.
Outdated
echo "PASS: group has a run-unique (github.run_id) fallback in $WORKFLOW"
else
{
echo "FAIL: concurrency group has no run-unique (github.run_id) fallback in $WORKFLOW"
echo " An event with no PR (fork / no PR) must fall back to"
echo " format('pr-auto-review-ready-check-unique-{0}', github.run_id) so"
echo " unrelated work never collapses into one cancelable group."
} >&2
PASS=false
fi
fi

# ── Check 7: cancel-in-progress stays gated on check_suite / workflow_run ──
# Cancellation is only safe for the default-branch-context events (which do not
# attach to the PR head). It must NOT be unconditionally true, or a pull_request
# run on the PR head could be cancelled, leaving a cancelled check on the head.
cancel=""
if ! cancel=$(yq '.concurrency.cancel-in-progress' "$WORKFLOW" 2>/dev/null); then
echo "FAIL: yq failed to parse $WORKFLOW. Please check if it is valid YAML."
exit 1
fi
# Validate the whole expression, not just its identifiers: after stripping the
# ${{ }} wrapper, whitespace and redundant outer parens it must be exactly the
# OR of the two event-name equalities (either order). A contradictory '&&' is
# false for every event and would silently disable cancellation.
cancel_norm="${cancel//[[:space:]]/}"
cancel_norm="${cancel_norm#\$\{\{}"
cancel_norm="${cancel_norm%\}\}}"
Comment thread
don-petry marked this conversation as resolved.
cs_eq="github.event_name=='check_suite'"
wr_eq="github.event_name=='workflow_run'"
# Single-PR form: each event is additionally restricted to exactly one listed PR,
# so a multi-PR (run-unique group) run is never cancelled.
cs_one="(${cs_eq}&&github.event.check_suite.pull_requests[0]&&(github.event.check_suite.pull_requests[1]==null))"
cs_one_strict="(${cs_eq}&&github.event.check_suite.pull_requests[0]&&github.event.check_suite.pull_requests[0].number&&(github.event.check_suite.pull_requests[1]==null))"
wr_one="(${wr_eq}&&github.event.workflow_run.pull_requests[0]&&(github.event.workflow_run.pull_requests[1]==null))"
wr_one_strict="(${wr_eq}&&github.event.workflow_run.pull_requests[0]&&github.event.workflow_run.pull_requests[0].number&&(github.event.workflow_run.pull_requests[1]==null))"
cancel_plain="$cancel_norm"
while [[ "$cancel_plain" == \(*\) ]]; do
cancel_plain="${cancel_plain#\(}"
cancel_plain="${cancel_plain%\)}"
done
if [[ "$cancel_plain" == "${cs_eq}||${wr_eq}" || "$cancel_plain" == "${wr_eq}||${cs_eq}" \
|| "$cancel_norm" == "${cs_one}||${wr_one}" || "$cancel_norm" == "${wr_one}||${cs_one}" \
|| "$cancel_norm" == "${cs_one_strict}||${wr_one_strict}" || "$cancel_norm" == "${wr_one_strict}||${cs_one_strict}" ]]; then
echo "PASS: cancel-in-progress is gated on check_suite / workflow_run in $WORKFLOW"
else
{
echo "FAIL: 'concurrency.cancel-in-progress' (found: '$cancel') is not gated on the"
echo " check_suite / workflow_run events in $WORKFLOW"
echo " Use an expression such as:"
echo " \${{ github.event_name == 'check_suite' || github.event_name == 'workflow_run' }}"
echo " so pull_request-head runs are never cancelled."
} >&2
PASS=false
fi
Comment thread
don-petry marked this conversation as resolved.

echo ""
if [[ "$PASS" == "true" ]]; then
echo "All checks passed."
else
echo "One or more checks failed."
exit 1
fi
Loading
Loading