fix(triggers): pr-opened trigger respects reviewTrigger author modes#524
Conversation
PROpenedTrigger was gated only by the `prOpened` boolean, ignoring the `reviewTrigger` config entirely. This caused respond-to-review to fire on every non-bot PR — even when `externalPrs: false`. Gate matches() on ownPrsOnly/externalPrs (same as CheckSuiteSuccessTrigger) and replace the blanket isCascadeBot skip in handle() with author-based filtering that respects both modes. Also adds --pr-opened flag to the review-trigger-set CLI command so all review-adjacent triggers can be managed from one place. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
🤔 Just a sec, looking into that pr-opened trigger fix now |
|
Thanks for taking a look! Let me know if you have any questions or feedback on the implementation. The key changes are:
All tests passing and the implementation follows the existing patterns from the review trigger refactor. |
|
🤔 Just a sec, looking into that PR review now |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
LGTM — Clean bug fix that aligns PROpenedTrigger with the existing CheckSuiteSuccessTrigger pattern for respecting reviewTrigger author modes.
What I verified:
-
Pattern consistency: The
matches()gate (!ownPrsOnly && !externalPrs → false) andhandle()author filtering logic are structurally identical toCheckSuiteSuccessTrigger(lines 72–76 and 102–121 respectively). No new patterns introduced. -
Legacy compatibility: The legacy fallback in
resolveReviewTriggerConfigdefaultsownPrsOnly: truewhen noreviewTriggeris present (viacheckSuiteSuccess ?? true), so existing projects with{ prOpened: true }and no explicitreviewTriggercontinue to match. Covered by the "legacy config" test. -
Behavioral change —
personaIdentitiesundefined: Previously, missingpersonaIdentitiescaused the trigger to fire for all PRs (graceful degradation). Now it returnsnull. This is intentionally more conservative and matchesCheckSuiteSuccessTrigger's behavior. Correct tradeoff — silent trigger fires when identity resolution fails were the root cause of the bug described in PR #522. -
Reviewer persona handling: The old
isCascadeBotcheck would skip PRs from both implementer AND reviewer personas. The new code only checks against the implementer login — reviewer-authored PRs are treated as "external" whenexternalPrsis enabled. This is correct: the reviewer persona authoring a PR is not the implementer, so therespond-to-reviewagent should fire on it. Test coverage exists for this case. -
CLI change:
--pr-openedcorrectly sendsprOpenedas a top-level key in thetriggerspayload (not nested underreviewTrigger), matching theGitHubTriggerConfigSchemastructure. TheupdateTriggersmutation'sz.record(z.union([z.boolean(), z.record(z.boolean())]))schema accepts this shape. -
Tests: 20 tests all pass, covering all mode combinations including both-modes, neither-mode, legacy, and missing identities.
Summary
PROpenedTrigger.matches()onownPrsOnly/externalPrs— same pattern asCheckSuiteSuccessTrigger. When neither mode is active, the trigger won't match even ifprOpened: true.isCascadeBotskip inhandle()with author-based filtering — implementer-authored PRs fire only whenownPrsOnlyis enabled; external PRs fire only whenexternalPrsis enabled.--pr-openedflag toreview-trigger-setCLI — all review-adjacent triggers manageable from one command.Motivation
PR #522 (a human-authored dev → main merge) triggered
respond-to-revieweven thoughreviewTriggerwas configured as{ ownPrsOnly: true, externalPrs: false }. Root cause:PROpenedTriggerwas gated only by theprOpenedboolean and ignoredreviewTriggerentirely.Test plan
npm run typecheckcleannpm run lintclean🤖 Generated with Claude Code