From 9c48eeb8ee4f0d2c095957f493f19d0d355bff9e Mon Sep 17 00:00:00 2001 From: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Date: Sun, 2 Aug 2026 10:10:29 +0000 Subject: [PATCH 1/2] =?UTF-8?q?feat:=20implement=20issue=20#1417=20?= =?UTF-8?q?=E2=80=94=20[bug]=20dev-lead=20reviewer-trust=20gate=20reads=20?= =?UTF-8?q?the=20PR=20author's=20association,=20not=20the=20reviewer's=20?= =?UTF-8?q?=E2=80=94=20untrusted-reviewer=20skip=20is=20dead=20code?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- scripts/dev-lead-intent.sh | 10 +++- .../events/pr_review_human_owner.json | 1 + .../pr_review_reviewer_missing_assoc.json | 35 +++++++++++++ .../pr_review_reviewer_trusted_bot.json | 36 +++++++++++++ .../pr_review_reviewer_trusted_human.json | 36 +++++++++++++ .../events/pr_review_reviewer_untrusted.json | 36 +++++++++++++ tests/dev-lead/unit/test_intent_reviews.bats | 52 +++++++++++++++++++ 7 files changed, 204 insertions(+), 2 deletions(-) create mode 100644 tests/dev-lead/fixtures/events/pr_review_reviewer_missing_assoc.json create mode 100644 tests/dev-lead/fixtures/events/pr_review_reviewer_trusted_bot.json create mode 100644 tests/dev-lead/fixtures/events/pr_review_reviewer_trusted_human.json create mode 100644 tests/dev-lead/fixtures/events/pr_review_reviewer_untrusted.json diff --git a/scripts/dev-lead-intent.sh b/scripts/dev-lead-intent.sh index 7532b80a5..ced17f1d4 100755 --- a/scripts/dev-lead-intent.sh +++ b/scripts/dev-lead-intent.sh @@ -258,7 +258,13 @@ case "$EVENT_NAME" in review_state=$(jq -r '.review.state // empty' "$EVENT_PATH" 2>/dev/null || true) pr_number=$(jq -r '.pull_request.number // empty' "$EVENT_PATH" 2>/dev/null || true) head_sha=$(jq -r '.pull_request.head.sha // empty' "$EVENT_PATH" 2>/dev/null || true) - author_assoc=$(jq -r '.pull_request.author_association // empty' "$EVENT_PATH" 2>/dev/null || true) + # Reviewer-trust gate (#1417): read the REVIEWER's association from .review, + # not the PR author's from .pull_request. On a dev-lead-authored PR the author + # is always dev-lead's own OWNER identity, so gating on the PR author made + # is_human_trusted always true and the untrusted-reviewer skip dead code. + # `// empty` keeps the gate fail-closed: a missing/unparseable reviewer + # association is untrusted (is_human_trusted returns false → skip). + reviewer_assoc=$(jq -r '.review.author_association // empty' "$EVENT_PATH" 2>/dev/null || true) # Skip if actor is BOT_USER (self-review) if [ "$reviewer" = "$BOT_USER" ]; then @@ -296,7 +302,7 @@ case "$EVENT_NAME" in else emit_intent "fix-reviews" "bot-review-${review_state}" "$context" fi - elif is_human_trusted "$author_assoc"; then + elif is_human_trusted "$reviewer_assoc"; then emit_intent "review-changes" "human-review-${review_state}" "$context" else emit_skip "untrusted-reviewer" diff --git a/tests/dev-lead/fixtures/events/pr_review_human_owner.json b/tests/dev-lead/fixtures/events/pr_review_human_owner.json index 3bfb100d3..f657dbc3e 100644 --- a/tests/dev-lead/fixtures/events/pr_review_human_owner.json +++ b/tests/dev-lead/fixtures/events/pr_review_human_owner.json @@ -5,6 +5,7 @@ "id": 1004, "state": "CHANGES_REQUESTED", "body": "Please refactor the authentication logic and add more tests.", + "author_association": "OWNER", "user": { "login": "donpetry", "type": "User" diff --git a/tests/dev-lead/fixtures/events/pr_review_reviewer_missing_assoc.json b/tests/dev-lead/fixtures/events/pr_review_reviewer_missing_assoc.json new file mode 100644 index 000000000..e60938cdd --- /dev/null +++ b/tests/dev-lead/fixtures/events/pr_review_reviewer_missing_assoc.json @@ -0,0 +1,35 @@ +{ + "_test_expected_intent": "skip", + "_test_note": "Reviewer-trust gate fail-closed case (#1417, AC #4): PR author held at OWNER, human reviewer with NO .review.author_association field. A missing/unparseable reviewer association must be treated as untrusted → skip untrusted-reviewer, never as trusted.", + "action": "submitted", + "review": { + "id": 5004, + "state": "CHANGES_REQUESTED", + "body": "Missing association on this review.", + "user": { + "login": "unknown-reviewer", + "type": "User" + } + }, + "pull_request": { + "number": 1417, + "title": "fix: reviewer-trust gate", + "state": "open", + "author_association": "OWNER", + "user": { + "login": "donpetry-bot", + "type": "Bot" + }, + "head": { + "sha": "aaa111bbb222", + "ref": "dev-lead/issue-1417-20260802-0956", + "repo": { "full_name": "petry-projects/.github-private" } + }, + "base": { + "ref": "main", + "repo": { "full_name": "petry-projects/.github-private" } + } + }, + "repository": { "full_name": "petry-projects/.github-private" }, + "sender": { "login": "unknown-reviewer", "type": "User" } +} diff --git a/tests/dev-lead/fixtures/events/pr_review_reviewer_trusted_bot.json b/tests/dev-lead/fixtures/events/pr_review_reviewer_trusted_bot.json new file mode 100644 index 000000000..dcf59a14f --- /dev/null +++ b/tests/dev-lead/fixtures/events/pr_review_reviewer_trusted_bot.json @@ -0,0 +1,36 @@ +{ + "_test_expected_intent": "fix-reviews", + "_test_note": "Reviewer-trust gate (#1417): PR author held at OWNER, reviewer is a trusted bot whose review association is NONE. The bot path keys on the reviewer login, not the association, so it still routes fix-reviews — no regression from the association fix.", + "action": "submitted", + "review": { + "id": 5003, + "state": "CHANGES_REQUESTED", + "body": "Several issues need to be addressed.", + "author_association": "NONE", + "user": { + "login": "gemini-code-assist[bot]", + "type": "Bot" + } + }, + "pull_request": { + "number": 1417, + "title": "fix: reviewer-trust gate", + "state": "open", + "author_association": "OWNER", + "user": { + "login": "donpetry-bot", + "type": "Bot" + }, + "head": { + "sha": "aaa111bbb222", + "ref": "dev-lead/issue-1417-20260802-0956", + "repo": { "full_name": "petry-projects/.github-private" } + }, + "base": { + "ref": "main", + "repo": { "full_name": "petry-projects/.github-private" } + } + }, + "repository": { "full_name": "petry-projects/.github-private" }, + "sender": { "login": "gemini-code-assist[bot]", "type": "Bot" } +} diff --git a/tests/dev-lead/fixtures/events/pr_review_reviewer_trusted_human.json b/tests/dev-lead/fixtures/events/pr_review_reviewer_trusted_human.json new file mode 100644 index 000000000..c3089ed65 --- /dev/null +++ b/tests/dev-lead/fixtures/events/pr_review_reviewer_trusted_human.json @@ -0,0 +1,36 @@ +{ + "_test_expected_intent": "review-changes", + "_test_note": "Reviewer-trust gate (#1417): PR author held at OWNER, reviewer association COLLABORATOR → the reviewer is a trusted human, so route review-changes. Distinguishes the reviewer's association from the PR author's.", + "action": "submitted", + "review": { + "id": 5001, + "state": "CHANGES_REQUESTED", + "body": "Please tighten the error handling here.", + "author_association": "COLLABORATOR", + "user": { + "login": "trusted-maintainer", + "type": "User" + } + }, + "pull_request": { + "number": 1417, + "title": "fix: reviewer-trust gate", + "state": "open", + "author_association": "OWNER", + "user": { + "login": "donpetry-bot", + "type": "Bot" + }, + "head": { + "sha": "aaa111bbb222", + "ref": "dev-lead/issue-1417-20260802-0956", + "repo": { "full_name": "petry-projects/.github-private" } + }, + "base": { + "ref": "main", + "repo": { "full_name": "petry-projects/.github-private" } + } + }, + "repository": { "full_name": "petry-projects/.github-private" }, + "sender": { "login": "trusted-maintainer", "type": "User" } +} diff --git a/tests/dev-lead/fixtures/events/pr_review_reviewer_untrusted.json b/tests/dev-lead/fixtures/events/pr_review_reviewer_untrusted.json new file mode 100644 index 000000000..e94ab0749 --- /dev/null +++ b/tests/dev-lead/fixtures/events/pr_review_reviewer_untrusted.json @@ -0,0 +1,36 @@ +{ + "_test_expected_intent": "skip", + "_test_note": "Reviewer-trust gate regression guard (#1417): PR author held at OWNER while reviewer association is CONTRIBUTOR (untrusted). Under the pre-fix code (which read .pull_request.author_association == OWNER) this routed review-changes; the fixed gate reads .review.author_association == CONTRIBUTOR and emits skip untrusted-reviewer.", + "action": "submitted", + "review": { + "id": 5002, + "state": "CHANGES_REQUESTED", + "body": "I think this needs work.", + "author_association": "CONTRIBUTOR", + "user": { + "login": "outside-contributor", + "type": "User" + } + }, + "pull_request": { + "number": 1417, + "title": "fix: reviewer-trust gate", + "state": "open", + "author_association": "OWNER", + "user": { + "login": "donpetry-bot", + "type": "Bot" + }, + "head": { + "sha": "aaa111bbb222", + "ref": "dev-lead/issue-1417-20260802-0956", + "repo": { "full_name": "petry-projects/.github-private" } + }, + "base": { + "ref": "main", + "repo": { "full_name": "petry-projects/.github-private" } + } + }, + "repository": { "full_name": "petry-projects/.github-private" }, + "sender": { "login": "outside-contributor", "type": "User" } +} diff --git a/tests/dev-lead/unit/test_intent_reviews.bats b/tests/dev-lead/unit/test_intent_reviews.bats index d7d0d1fd2..ae6822adb 100644 --- a/tests/dev-lead/unit/test_intent_reviews.bats +++ b/tests/dev-lead/unit/test_intent_reviews.bats @@ -180,6 +180,58 @@ EOF [ "$(_get_env INTENT_TYPE)" = "fix-reviews" ] } +# ── reviewer-trust gate: gate on the REVIEWER's association (#1417) ─────────── +# The pull_request_review branch must evaluate the reviewer's association +# (.review.author_association), NOT the PR author's (.pull_request.author_association). +# Every fixture below holds .pull_request.author_association at OWNER (as it always +# is on a dev-lead-authored PR) and varies only .review.author_association, so each +# assertion would fail against the pre-fix code that read the PR author's field. + +@test "reviews: reviewer-trust — trusted-human reviewer (COLLABORATOR) → review-changes" { + export GITHUB_EVENT_NAME="pull_request_review" + export GITHUB_EVENT_PATH="$FIXTURES_DIR/pr_review_reviewer_trusted_human.json" + + run bash "$INTENT_SCRIPT" + + [ "$status" -eq 0 ] + [ "$(_get_env INTENT_TYPE)" = "review-changes" ] +} + +@test "reviews: reviewer-trust — untrusted reviewer (CONTRIBUTOR) on OWNER-authored PR → skip untrusted-reviewer" { + # Regression guard (#1417): under the pre-fix code this read .pull_request.author_association + # (OWNER) and routed review-changes — the untrusted-reviewer skip was dead code. The fixed + # gate reads .review.author_association (CONTRIBUTOR) and reaches the skip. + export GITHUB_EVENT_NAME="pull_request_review" + export GITHUB_EVENT_PATH="$FIXTURES_DIR/pr_review_reviewer_untrusted.json" + + run bash "$INTENT_SCRIPT" + + [ "$status" -eq 0 ] + [ "$(_get_env INTENT_TYPE)" = "skip" ] + [ "$(_get_env INTENT_REASON)" = "untrusted-reviewer" ] +} + +@test "reviews: reviewer-trust — trusted bot (assoc NONE) → fix-reviews (bot path ignores assoc)" { + export GITHUB_EVENT_NAME="pull_request_review" + export GITHUB_EVENT_PATH="$FIXTURES_DIR/pr_review_reviewer_trusted_bot.json" + + run bash "$INTENT_SCRIPT" + + [ "$status" -eq 0 ] + [ "$(_get_env INTENT_TYPE)" = "fix-reviews" ] +} + +@test "reviews: reviewer-trust — missing reviewer association fails closed → skip untrusted-reviewer" { + export GITHUB_EVENT_NAME="pull_request_review" + export GITHUB_EVENT_PATH="$FIXTURES_DIR/pr_review_reviewer_missing_assoc.json" + + run bash "$INTENT_SCRIPT" + + [ "$status" -eq 0 ] + [ "$(_get_env INTENT_TYPE)" = "skip" ] + [ "$(_get_env INTENT_REASON)" = "untrusted-reviewer" ] +} + # ── pull_request_review_comment tests ──────────────────────────────────────── @test "reviews: pull_request_review_comment codex → fix-reviews" { From c2c43a2ba0c41f3c7142f4309a6497bc3af6d902 Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Sun, 2 Aug 2026 10:17:45 +0000 Subject: [PATCH 2/2] fix(reviews): address review comments [skip ci-relay] --- scripts/dev-lead-intent.sh | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/scripts/dev-lead-intent.sh b/scripts/dev-lead-intent.sh index ced17f1d4..822452bdf 100755 --- a/scripts/dev-lead-intent.sh +++ b/scripts/dev-lead-intent.sh @@ -262,9 +262,10 @@ case "$EVENT_NAME" in # not the PR author's from .pull_request. On a dev-lead-authored PR the author # is always dev-lead's own OWNER identity, so gating on the PR author made # is_human_trusted always true and the untrusted-reviewer skip dead code. - # `// empty` keeps the gate fail-closed: a missing/unparseable reviewer - # association is untrusted (is_human_trusted returns false → skip). - reviewer_assoc=$(jq -r '.review.author_association // empty' "$EVENT_PATH" 2>/dev/null || true) + # `.review?` + `// "" | tostring` keeps the gate fail-closed: a missing or + # null reviewer association produces an empty string that is_human_trusted + # rejects (returns false → skip). + reviewer_assoc=$(jq -r '(.review?.author_association // "" | tostring)' "$EVENT_PATH") # Skip if actor is BOT_USER (self-review) if [ "$reviewer" = "$BOT_USER" ]; then