Repository navigation
[bug] dev-lead reviewer-trust gate reads the PR author's association, not the reviewer's — untrusted-reviewer skip is dead code #1417
Description
Story
As a maintainer relying on dev-lead's reviewer-trust gate,
I want the gate to evaluate the reviewer's repo association rather than the PR author's,
so that "only a trusted human's review drives dev-lead's fix/push path" is actually enforced instead of silently always-true on dev-lead-authored PRs.
Problem
In the pull_request_review branch of scripts/dev-lead-intent.sh, the trust gate reads the wrong field:
# scripts/dev-lead-intent.sh:261
author_assoc=$(jq -r '.pull_request.author_association // empty' "$EVENT_PATH" ...)
...
# scripts/dev-lead-intent.sh:~300
elif is_human_trusted "$author_assoc"; then
emit_intent "review-changes" "human-review-${review_state}" "$context"
else
emit_skip "untrusted-reviewer"
fi.pull_request.author_association is the PR author's association with the repo. The reviewer's association is .review.author_association. So is_human_trusted — whose evident intent is "is this reviewer a trusted human" — is actually asking "is the PR author trusted."
Consequence. The branch is only reached after the is_dev_lead_authored gate (#1311), so the PR author is always dev-lead's own identity (don-petry, OWNER). is_human_trusted is therefore always true on that path:
- Every non-trusted-bot reviewer routes to
review-changes, whatever their actual association. - The
untrusted-reviewerskip is dead code on the very path it exists to protect.
This is inconsistent with the sibling branches, which get it right — issue_comment (:316) and pull_request_review_comment (:367) both read .comment.author_association, i.e. the commenter's association. Only the review branch reads from .pull_request.
Practical exposure is currently bounded (the repo is private, so only users with access can review, and fork PRs are skipped earlier), which is why this is a correctness/defence-in-depth fix rather than an active incident. But a trust gate that never rejects is not a trust gate — the same class of defect as #1415 (a gate whose control is not actually load-bearing) and #860 normative rule 2.
Acceptance Criteria
- The
pull_request_reviewbranch evaluates the reviewer's association, sourced from.review.author_association, not.pull_request.author_association. - A review from an author_association outside
OWNER|MEMBER|COLLABORATORon a dev-lead-authored PR emitsskipwith reasonuntrusted-reviewer— i.e. the skip branch becomes reachable and is proven reachable by a test. - A review from a trusted human (
OWNER/MEMBER/COLLABORATOR) still routes toreview-changes, and a trusted-bot review still routes tofix-reviews/enable-auto-merge— no regression to the existing paths. - Fails closed: a missing or unparseable reviewer association is treated as untrusted (skip), never as trusted.
- Bats coverage with a fixture per case: trusted-human reviewer, untrusted reviewer (the currently-impossible case — the regression guard), trusted bot, missing association. Fixtures assert the reviewer's association is what moves the decision, e.g. by holding the PR author constant at
OWNERwhile varying.review.author_association. - The three branches are checked for consistency:
pull_request_review,issue_comment, andpull_request_review_commentall gate on the association of the actor who produced the event.
Tasks / Subtasks
- Change the
author_assocassignment in thepull_request_reviewbranch to read.review.author_association; consider renaming the local toreviewer_assocso the sibling branches'author_assoc(commenter) is not confused with it. (AC: test issue from agent #1, feat: feature ideation framework — adversarial proposer/challenger/synthesizer #6) - Add the fail-closed default for a missing/empty association. (AC: Optimize review: small-PR and incremental fast paths #4)
- Add bats fixtures + assertions for all four cases, holding the PR author constant so the test would fail against today's code. (AC: Go-live improvements for PR review agent #2, Add @claude delegation, auto-merge, and rebase handling #3, feat: add Copilot engine support via REVIEW_ENGINE toggle #5)
- Re-check the two comment branches for the same class of mistake and note the result in the PR. (AC: feat: feature ideation framework — adversarial proposer/challenger/synthesizer #6)
Dev Notes
- Test-design note (the point of AC feat: add Copilot engine support via REVIEW_ENGINE toggle #5): a fixture that varies both the PR author and the reviewer cannot distinguish the bug from correct behaviour. Hold
.pull_request.author_associationatOWNER(as it always is on a dev-lead PR) and vary only.review.author_association— that is the assertion that fails today and passes after the fix. - Interaction with [Phase 4] Close the maintainer review-gate gap — dev-lead PRs cannot be blocked by the owner (shared identity + agent-resolved threads) #1415 and with real review workflows. Once this gate actually evaluates the reviewer, reviews from accounts below
COLLABORATORstop driving dev-lead. That is the intended behaviour, but it is a live behavioural change worth calling out in the PR: e.g. a read-only second identity used by a maintainer to post a blockingCHANGES_REQUESTED(the workaround for [Phase 4] Close the maintainer review-gate gap — dev-lead PRs cannot be blocked by the owner (shared identity + agent-resolved threads) #1415) may resolve to an association that this gate rejects, so dev-lead would block-but-not-act. Confirm the association such an account actually reports before landing, and reconcile with [Phase 4] Close the maintainer review-gate gap — dev-lead PRs cannot be blocked by the owner (shared identity + agent-resolved threads) #1415's design so the two do not fight. - Keep the existing ordering intact — the
BOT_USERself-review skip, the fork-PR skip, and theis_dev_lead_authoredauthorship gate (dev-lead fixes-and-merges PRs it did NOT author — races the human driver (dropped a commit) #1311) all run before this branch and must continue to.
Project Structure Notes
scripts/dev-lead-intent.sh (the pull_request_review branch) plus its bats suite under tests/dev-lead/unit/; fixtures alongside the existing intent fixtures.
References
scripts/dev-lead-intent.sh:256-302(the affected branch),:316,:367(the correct sibling branches)is_human_trusted()in the same file- dev-lead fixes-and-merges PRs it did NOT author — races the human driver (dropped a commit) #1311 (the authorship gate this branch sits behind)
- [Phase 4] Close the maintainer review-gate gap — dev-lead PRs cannot be blocked by the owner (shared identity + agent-resolved threads) #1415 (the sibling gate-integrity gap on the same review path)
docs/postmortems/2026-06-pr-860-runaway.md(rule 2 — a control that cannot reject is not a control)
Likely target surface
scripts/dev-lead-intent.sh
Found while verifying whether a review posted from a non-don-petry identity would be honoured by dev-lead (the #1415 workaround). Filed under epic #1402.
Activity
github-actions commentedon Aug 2, 2026
Thank you for the thorough write-up — the field-level diff between
.pull_request.author_associationand.review.author_association, the always-true analysis, and the dead-code consequence are all clearly laid out and make triage straightforward. Before this lands, the dev notes flag one open question worth confirming up front: does the secondary identity you tested as a #1415 workaround resolve to an association withinOWNER|MEMBER|COLLABORATOR, or would the corrected gate reject its reviews and leave dev-lead in a block-but-not-act state on that path?don-petry commentedon Aug 2, 2026
Dev-Lead Implementation Plan
Issue: #1417 — [bug] dev-lead reviewer-trust gate reads the PR author's association, not the reviewer's — untrusted-reviewer skip is dead code
Scope
Fix the
pull_request_reviewbranch ofscripts/dev-lead-intent.shso the reviewer-trust gate evaluates the reviewer's association (.review.author_association) instead of the PR author's (.pull_request.author_association), making theuntrusted-reviewerskip reachable and fail-closed.Implementation Checklist
scripts/dev-lead-intent.sh: in thepull_request_reviewbranch, source the association from.review.author_associationand rename the localauthor_assoc→reviewer_assocso it is not confused with the commenter'sauthor_associn the sibling branches (AC test issue from agent #1, feat: feature ideation framework — adversarial proposer/challenger/synthesizer #6)skip untrusted-reviewer(AC Optimize review: small-PR and incremental fast paths #4)issue_comment,pull_request_review_comment) — they already read.comment.author_association(the actor); note the confirmation in the PR (AC feat: feature ideation framework — adversarial proposer/challenger/synthesizer #6)tests/dev-lead/fixtures/events/pr_review_human_owner.jsonto carry.review.author_associationso the trusted-human case still routes toreview-changesunder the fixed gateTests to Write
review-changesskip untrusted-reviewer— the regression guard that fails against today's codefix-reviews— proves the bot path ignores assoc, no regression.review.author_associationabsent) →skip untrusted-reviewer— fail-closed.pull_request.author_associationat OWNER and vary only.review.author_association, so each test would fail against the pre-fix codeTest Command
bats tests/dev-lead/unit/(CI:.github/workflows/test-dev-lead.yml), plusbash .dev-lead/scripts/dev-lead-lint.shdon-petry commentedon Aug 2, 2026
Dev-Lead: Implementation Complete
Plan Execution
scripts/dev-lead-intent.sh—pull_request_reviewbranch now sources the association from.review.author_association(the reviewer), renamedauthor_assoc→reviewer_assocso it is no longer confused with the commenter'sauthor_associn the two sibling branches (AC test issue from agent #1, feat: feature ideation framework — adversarial proposer/challenger/synthesizer #6)// emptyplusis_human_trusted's default-falsecase means a missing/unparseable reviewer association resolves toskip untrusted-reviewer, never trusted (AC Optimize review: small-PR and incremental fast paths #4)pull_request_review_comment(:322) andissue_comment(:373) already read.comment.author_association(the commenter). All three branches now gate on the association of the actor who produced the event (AC feat: feature ideation framework — adversarial proposer/challenger/synthesizer #6).pull_request.author_associationatOWNERand varying only.review.author_association, so each test would fail against the pre-fix code (AC feat: add Copilot engine support via REVIEW_ENGINE toggle #5)Test Results
Files Changed
scripts/dev-lead-intent.sh: reviewer-trust gate reads.review.author_association(reviewer), local renamed toreviewer_assoc, fail-closed comment addedtests/dev-lead/unit/test_intent_reviews.bats: new "reviewer-trust gate" section with the four casestests/dev-lead/fixtures/events/pr_review_human_owner.json: added.review.author_association: OWNERso the trusted-human case stays valid under the fixed gatetests/dev-lead/fixtures/events/pr_review_reviewer_trusted_human.json(new): PR author OWNER, reviewer COLLABORATOR → review-changestests/dev-lead/fixtures/events/pr_review_reviewer_untrusted.json(new): PR author OWNER, reviewer CONTRIBUTOR → skip untrusted-reviewer (the regression guard)tests/dev-lead/fixtures/events/pr_review_reviewer_trusted_bot.json(new): PR author OWNER, trusted bot with assoc NONE → fix-reviewstests/dev-lead/fixtures/events/pr_review_reviewer_missing_assoc.json(new): PR author OWNER, no.review.author_association→ skip untrusted-reviewer (fail-closed)Notes
COLLABORATORno longer drives dev-lead's fix/push path — it emitsskip untrusted-reviewer. This is the intended behaviour and the whole point of the fix. The self-review, fork-PR, andis_dev_lead_authored(dev-lead fixes-and-merges PRs it did NOT author — races the human driver (dropped a commit) #1311) gates all still run before this branch, unchanged.don-petry/OWNER, per AGENTS.md "Agent identity & credential secrets"), landed in feat: implement issue #1415 — [Phase 4] Close the maintainer review-gate gap — dev-lead PRs cannot be blocked by the owner (shared identity + agent-resolved threads) #1433. A maintainer blocking review from the owner identity resolves toOWNERand is honoured by this gate, so the two do not fight. The earlier read-only-second-identity workaround (which could resolve belowCOLLABORATORand be rejected here) is superseded by that shared-identity design; no separate reconciliation needed. Worth a reviewer's eye all the same.feat: implement issue #1417 — [bug] dev-lead reviewer-trust gate read…