diff --git a/generator/src/tend/templates/mention.yaml.j2 b/generator/src/tend/templates/mention.yaml.j2 index 124e3c04..b81f8665 100644 --- a/generator/src/tend/templates/mention.yaml.j2 +++ b/generator/src/tend/templates/mention.yaml.j2 @@ -209,8 +209,10 @@ jobs: # comments on purpose: the pull_request_review *submission* kind is # deliberately left out, since a review the bot leaves on its own PR # is its reviewer role (the prompt is told to action it), not a - # self-loop. The only self-review skip is the terminal empty-body - # APPROVED gate below. + # self-loop. The two self-review skips below are author-keyed, but + # each is also narrowed to a case that leaves this run nothing to do: + # the empty-body APPROVED gate, and a bot review on a PR the bot did + # not author. Neither licenses a blanket self-review skip. if { [ "$KIND" = "issue_comment" ] || [ "$KIND" = "pull_request_review_comment" ]; } \ && [ "$COMMENT_AUTHOR" = "<>" ]; then echo "should_run=false" >> "$GITHUB_OUTPUT" @@ -304,6 +306,26 @@ jobs: echo "reason=participation" >> "$GITHUB_OUTPUT"; exit 0 fi + # A review the bot leaves on someone else's PR leaves this run + # nothing to do: whatever the review warranted, the tend-review + # session that submitted it has already done — left the findings for + # a human author to act on (pushing to their branch unbidden is + # barred by conduct rules), or, on a dependency-bot PR where no + # author will act, pushed the fix itself. The BOT_REVIEWS heuristic + # below counts this very review, so without this gate the session + # always starts and always exits silently. Keyed on author alone — + # the APPROVED + empty-body gate above is the same shape narrowed to + # its one terminal leg, and reusing those clauses here would let every + # bodied COMMENTED review through. Placement carries the rest of the + # design: *after* the PR_AUTHOR short-circuit, which has already + # exited when the PR is the bot's own, so the reviewer-to-author + # handoff on a bot PR still fires; *after* the body and inline + # @-mention scans, so an explicit summons still wins. + if [ "$KIND" = "pull_request_review" ] \ + && [ "$REVIEW_AUTHOR" = "<>" ]; then + echo "should_run=false" >> "$GITHUB_OUTPUT"; exit 0 + fi + # Captured, not counted — see the note on the issue-comment lookup above. BOT_REVIEWS=$(gh api --paginate "repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/reviews" \ --jq '.[] | select(.user.login == "<>") | .id') diff --git a/generator/tests/_regtest_outputs/test_generate.test_sandbox_levers_regtest.out b/generator/tests/_regtest_outputs/test_generate.test_sandbox_levers_regtest.out index 1194c7c8..ebdcbe6a 100644 --- a/generator/tests/_regtest_outputs/test_generate.test_sandbox_levers_regtest.out +++ b/generator/tests/_regtest_outputs/test_generate.test_sandbox_levers_regtest.out @@ -195,8 +195,10 @@ jobs: # comments on purpose: the pull_request_review *submission* kind is # deliberately left out, since a review the bot leaves on its own PR # is its reviewer role (the prompt is told to action it), not a - # self-loop. The only self-review skip is the terminal empty-body - # APPROVED gate below. + # self-loop. The two self-review skips below are author-keyed, but + # each is also narrowed to a case that leaves this run nothing to do: + # the empty-body APPROVED gate, and a bot review on a PR the bot did + # not author. Neither licenses a blanket self-review skip. if { [ "$KIND" = "issue_comment" ] || [ "$KIND" = "pull_request_review_comment" ]; } \ && [ "$COMMENT_AUTHOR" = "test-bot" ]; then echo "should_run=false" >> "$GITHUB_OUTPUT" @@ -290,6 +292,26 @@ jobs: echo "reason=participation" >> "$GITHUB_OUTPUT"; exit 0 fi + # A review the bot leaves on someone else's PR leaves this run + # nothing to do: whatever the review warranted, the tend-review + # session that submitted it has already done ? left the findings for + # a human author to act on (pushing to their branch unbidden is + # barred by conduct rules), or, on a dependency-bot PR where no + # author will act, pushed the fix itself. The BOT_REVIEWS heuristic + # below counts this very review, so without this gate the session + # always starts and always exits silently. Keyed on author alone ? + # the APPROVED + empty-body gate above is the same shape narrowed to + # its one terminal leg, and reusing those clauses here would let every + # bodied COMMENTED review through. Placement carries the rest of the + # design: *after* the PR_AUTHOR short-circuit, which has already + # exited when the PR is the bot's own, so the reviewer-to-author + # handoff on a bot PR still fires; *after* the body and inline + # @-mention scans, so an explicit summons still wins. + if [ "$KIND" = "pull_request_review" ] \ + && [ "$REVIEW_AUTHOR" = "test-bot" ]; then + echo "should_run=false" >> "$GITHUB_OUTPUT"; exit 0 + fi + # Captured, not counted ? see the note on the issue-comment lookup above. BOT_REVIEWS=$(gh api --paginate "repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/reviews" \ --jq '.[] | select(.user.login == "test-bot") | .id') diff --git a/generator/tests/_regtest_outputs/test_generate.test_workflow_minimal_codex_regtest[mention].out b/generator/tests/_regtest_outputs/test_generate.test_workflow_minimal_codex_regtest[mention].out index 362efd9d..575544d3 100644 --- a/generator/tests/_regtest_outputs/test_generate.test_workflow_minimal_codex_regtest[mention].out +++ b/generator/tests/_regtest_outputs/test_generate.test_workflow_minimal_codex_regtest[mention].out @@ -195,8 +195,10 @@ jobs: # comments on purpose: the pull_request_review *submission* kind is # deliberately left out, since a review the bot leaves on its own PR # is its reviewer role (the prompt is told to action it), not a - # self-loop. The only self-review skip is the terminal empty-body - # APPROVED gate below. + # self-loop. The two self-review skips below are author-keyed, but + # each is also narrowed to a case that leaves this run nothing to do: + # the empty-body APPROVED gate, and a bot review on a PR the bot did + # not author. Neither licenses a blanket self-review skip. if { [ "$KIND" = "issue_comment" ] || [ "$KIND" = "pull_request_review_comment" ]; } \ && [ "$COMMENT_AUTHOR" = "test-bot" ]; then echo "should_run=false" >> "$GITHUB_OUTPUT" @@ -290,6 +292,26 @@ jobs: echo "reason=participation" >> "$GITHUB_OUTPUT"; exit 0 fi + # A review the bot leaves on someone else's PR leaves this run + # nothing to do: whatever the review warranted, the tend-review + # session that submitted it has already done ? left the findings for + # a human author to act on (pushing to their branch unbidden is + # barred by conduct rules), or, on a dependency-bot PR where no + # author will act, pushed the fix itself. The BOT_REVIEWS heuristic + # below counts this very review, so without this gate the session + # always starts and always exits silently. Keyed on author alone ? + # the APPROVED + empty-body gate above is the same shape narrowed to + # its one terminal leg, and reusing those clauses here would let every + # bodied COMMENTED review through. Placement carries the rest of the + # design: *after* the PR_AUTHOR short-circuit, which has already + # exited when the PR is the bot's own, so the reviewer-to-author + # handoff on a bot PR still fires; *after* the body and inline + # @-mention scans, so an explicit summons still wins. + if [ "$KIND" = "pull_request_review" ] \ + && [ "$REVIEW_AUTHOR" = "test-bot" ]; then + echo "should_run=false" >> "$GITHUB_OUTPUT"; exit 0 + fi + # Captured, not counted ? see the note on the issue-comment lookup above. BOT_REVIEWS=$(gh api --paginate "repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/reviews" \ --jq '.[] | select(.user.login == "test-bot") | .id') diff --git a/generator/tests/_regtest_outputs/test_generate.test_workflow_minimal_regtest[mention].out b/generator/tests/_regtest_outputs/test_generate.test_workflow_minimal_regtest[mention].out index f70efc1b..b5352752 100644 --- a/generator/tests/_regtest_outputs/test_generate.test_workflow_minimal_regtest[mention].out +++ b/generator/tests/_regtest_outputs/test_generate.test_workflow_minimal_regtest[mention].out @@ -195,8 +195,10 @@ jobs: # comments on purpose: the pull_request_review *submission* kind is # deliberately left out, since a review the bot leaves on its own PR # is its reviewer role (the prompt is told to action it), not a - # self-loop. The only self-review skip is the terminal empty-body - # APPROVED gate below. + # self-loop. The two self-review skips below are author-keyed, but + # each is also narrowed to a case that leaves this run nothing to do: + # the empty-body APPROVED gate, and a bot review on a PR the bot did + # not author. Neither licenses a blanket self-review skip. if { [ "$KIND" = "issue_comment" ] || [ "$KIND" = "pull_request_review_comment" ]; } \ && [ "$COMMENT_AUTHOR" = "test-bot" ]; then echo "should_run=false" >> "$GITHUB_OUTPUT" @@ -290,6 +292,26 @@ jobs: echo "reason=participation" >> "$GITHUB_OUTPUT"; exit 0 fi + # A review the bot leaves on someone else's PR leaves this run + # nothing to do: whatever the review warranted, the tend-review + # session that submitted it has already done ? left the findings for + # a human author to act on (pushing to their branch unbidden is + # barred by conduct rules), or, on a dependency-bot PR where no + # author will act, pushed the fix itself. The BOT_REVIEWS heuristic + # below counts this very review, so without this gate the session + # always starts and always exits silently. Keyed on author alone ? + # the APPROVED + empty-body gate above is the same shape narrowed to + # its one terminal leg, and reusing those clauses here would let every + # bodied COMMENTED review through. Placement carries the rest of the + # design: *after* the PR_AUTHOR short-circuit, which has already + # exited when the PR is the bot's own, so the reviewer-to-author + # handoff on a bot PR still fires; *after* the body and inline + # @-mention scans, so an explicit summons still wins. + if [ "$KIND" = "pull_request_review" ] \ + && [ "$REVIEW_AUTHOR" = "test-bot" ]; then + echo "should_run=false" >> "$GITHUB_OUTPUT"; exit 0 + fi + # Captured, not counted ? see the note on the issue-comment lookup above. BOT_REVIEWS=$(gh api --paginate "repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/reviews" \ --jq '.[] | select(.user.login == "test-bot") | .id') diff --git a/generator/tests/_regtest_outputs/test_generate.test_workflow_with_setup_regtest[mention].out b/generator/tests/_regtest_outputs/test_generate.test_workflow_with_setup_regtest[mention].out index da926bd5..e6f94bb0 100644 --- a/generator/tests/_regtest_outputs/test_generate.test_workflow_with_setup_regtest[mention].out +++ b/generator/tests/_regtest_outputs/test_generate.test_workflow_with_setup_regtest[mention].out @@ -195,8 +195,10 @@ jobs: # comments on purpose: the pull_request_review *submission* kind is # deliberately left out, since a review the bot leaves on its own PR # is its reviewer role (the prompt is told to action it), not a - # self-loop. The only self-review skip is the terminal empty-body - # APPROVED gate below. + # self-loop. The two self-review skips below are author-keyed, but + # each is also narrowed to a case that leaves this run nothing to do: + # the empty-body APPROVED gate, and a bot review on a PR the bot did + # not author. Neither licenses a blanket self-review skip. if { [ "$KIND" = "issue_comment" ] || [ "$KIND" = "pull_request_review_comment" ]; } \ && [ "$COMMENT_AUTHOR" = "test-bot" ]; then echo "should_run=false" >> "$GITHUB_OUTPUT" @@ -290,6 +292,26 @@ jobs: echo "reason=participation" >> "$GITHUB_OUTPUT"; exit 0 fi + # A review the bot leaves on someone else's PR leaves this run + # nothing to do: whatever the review warranted, the tend-review + # session that submitted it has already done ? left the findings for + # a human author to act on (pushing to their branch unbidden is + # barred by conduct rules), or, on a dependency-bot PR where no + # author will act, pushed the fix itself. The BOT_REVIEWS heuristic + # below counts this very review, so without this gate the session + # always starts and always exits silently. Keyed on author alone ? + # the APPROVED + empty-body gate above is the same shape narrowed to + # its one terminal leg, and reusing those clauses here would let every + # bodied COMMENTED review through. Placement carries the rest of the + # design: *after* the PR_AUTHOR short-circuit, which has already + # exited when the PR is the bot's own, so the reviewer-to-author + # handoff on a bot PR still fires; *after* the body and inline + # @-mention scans, so an explicit summons still wins. + if [ "$KIND" = "pull_request_review" ] \ + && [ "$REVIEW_AUTHOR" = "test-bot" ]; then + echo "should_run=false" >> "$GITHUB_OUTPUT"; exit 0 + fi + # Captured, not counted ? see the note on the issue-comment lookup above. BOT_REVIEWS=$(gh api --paginate "repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/reviews" \ --jq '.[] | select(.user.login == "test-bot") | .id') diff --git a/generator/tests/test_generate.py b/generator/tests/test_generate.py index cafefdb4..c8fd6d6b 100644 --- a/generator/tests/test_generate.py +++ b/generator/tests/test_generate.py @@ -1046,9 +1046,11 @@ def test_mention_self_comment_skip_spares_review_submissions( role speaking, not a self-loop — the prompt is told to action it. That signal arrives as a `pull_request_review` submission event, distinct from the `pull_request_review_comment` inline-comment event the skip covers, so - it must still reach the actionable path. The only self-review that is - skipped is the terminal empty-body APPROVED case (see - test_mention_skips_bot_approved_review). + it must still reach the actionable path. The self-reviews that are skipped + are skipped for being terminal for the bot, not for being self-authored: + the empty-body APPROVED case (see test_mention_skips_bot_approved_review) + and a review on a PR the bot did not author (see + test_mention_skips_bot_review_on_another_authors_pr). This pins the boundary against a later "skip self-authored reviews too, for consistency" edit that would silently break the review -> fix loop: the @@ -1081,18 +1083,83 @@ def test_mention_self_comment_skip_spares_review_submissions( "submission kind — a bot self-review is actionable reviewer signal" ) - # The sole author-keyed skip for a review submission is the terminal - # empty-body APPROVED gate; a COMMENTED / non-empty-body bot self-review - # falls through to the actionable PR_AUTHOR == bot short-circuit. - review_skip = run[run.index('[ "$KIND" = "pull_request_review" ]') :] - assert '[ "$REVIEW_STATE" = "approved" ]' in review_skip - assert '[ -z "$COMMENT_BODY" ]' in review_skip + # Slice to the terminal empty-body APPROVED gate alone — it ends at the + # inline-comment fetch, ahead of the other-authors-PR gate that + # test_mention_skips_bot_review_on_another_authors_pr pins — so this stays + # discriminating rather than passing on either. A COMMENTED / + # non-empty-body bot self-review that is neither falls through to the + # actionable PR_AUTHOR == bot short-circuit. + approved_skip = run[ + run.index('[ "$KIND" = "pull_request_review" ]') : run.index( + "reviews/$PAYLOAD_ID/comments" + ) + ] + assert '[ "$REVIEW_STATE" = "approved" ]' in approved_skip + assert '[ -z "$COMMENT_BODY" ]' in approved_skip assert '[ "$PR_AUTHOR" = "test-bot" ]' in run, ( "a bot self-review that isn't the terminal empty approval must reach " "the actionable PR_AUTHOR == bot short-circuit" ) +def test_mention_skips_bot_review_on_another_authors_pr(tmp_path: Path) -> None: + """A review the bot leaves on a PR someone else authored leaves the mention + run nothing to do: whatever the review warranted, the tend-review session + that submitted it has already done — left the findings for a human author + to act on (pushing to their branch unbidden is barred by conduct rules), + or, on a dependency-bot PR where no author will act, pushed the fix itself. + The BOT_REVIEWS heuristic counts this very review, so without a gate every + such review starts a session that can only exit silently (#915). #747's + gate covers only the empty-body APPROVED leg — + the reviews observed here carry 455-1920 char bodies in APPROVED and + COMMENTED states alike, so the gate must key on author alone. + + Placement is the whole design, in both directions: + + - *After* the PR_AUTHOR short-circuit, which exits should_run=true when the + PR is the bot's own. A review the bot leaves on its own PR is the reviewer + role handing work to the author role, and must keep firing (#166, #761) — + an author-keyed gate placed any earlier would swallow it. + - *After* the review-body and inline-comment @-mention scans, so an explicit + summons the bot quotes still wins. + - *Before* the BOT_REVIEWS heuristic, which is what misreads the triggering + review as prior engagement. + """ + cfg = Config.load(_minimal_config(tmp_path)) + wf = generate_mention(cfg) + data = yaml.safe_load(wf.content) + check_step = next( + s for s in data["jobs"]["verify"]["steps"] if s.get("id") == "check" + ) + run = check_step["run"] + + # The window between the PR-author resolution and the engagement heuristic + # is the only place the gate can sit and still satisfy all three orderings. + gate = run[run.index("PR_AUTHOR=$(gh pr view") : run.index("BOT_REVIEWS=$(")] + assert '[ "$KIND" = "pull_request_review" ]' in gate + assert '[ "$REVIEW_AUTHOR" = "test-bot" ]' in gate + + # Keyed on author alone. Reusing #747's state/body clauses here would let + # every review in #915's evidence through, since they are non-empty and + # mostly COMMENTED. + assert '[ "$REVIEW_STATE"' not in gate + assert '[ -z "$COMMENT_BODY" ]' not in gate + + # The bot's own PR still reaches the actionable short-circuit, because that + # check exits before the gate is read. + assert run.index('[ "$PR_AUTHOR" = "test-bot" ]') < run.index( + '[ "$KIND" = "pull_request_review" ]', run.index("PR_AUTHOR=$(gh pr view") + ) + + # And an @-mention inside the review — body or inline comment — still wins. + # Anchor the right-hand side to the gate itself, not to the heuristic below + # it: the inline fetch has always preceded BOT_REVIEWS, so that comparison + # would hold on the pre-change template and pin nothing. + assert run.index("reviews/$PAYLOAD_ID/comments") < run.index( + '[ "$REVIEW_AUTHOR" = "test-bot" ]', run.index("PR_AUTHOR=$(gh pr view") + ) + + # --------------------------------------------------------------------------- # Fork guard # ---------------------------------------------------------------------------