This is an automated email from the ASF dual-hosted git repository.
potiuk pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/airflow-steward.git
The following commit(s) were added to refs/heads/main by this push:
new dabb2d8 feat(pr-management-triage): collaborator-only thread check
for mark-ready + author-primary template polish (#226)
dabb2d8 is described below
commit dabb2d83d49a9163cb9d2133c984db997e9d00b3
Author: Jarek Potiuk <[email protected]>
AuthorDate: Tue May 19 03:46:07 2026 +0200
feat(pr-management-triage): collaborator-only thread check for mark-ready +
author-primary template polish (#226)
Two related improvements found while running pr-management-stats on
a large queue (apache/airflow, 477 open PRs):
**1. Row 20 (and row 19) — exclude non-collaborator unresolved
threads from the mark-ready / already-ready predicate.**
Previously rows 19/20 required `reviewThreads.totalCount == 0`
(any unresolved thread blocks mark-ready). In practice this caught
the "contributors review each other" case — drive-by reviewers,
copilot bot threads — and parked otherwise-passing PRs in an
unclassified limbo with no row matching them. They couldn't reach
the review queue.
The fix mirrors the `unresolved_threads_only` precondition (which
rows 14c/15 already use), where only collaborator-authored unresolved
threads count as a deterministic signal. Contributor-author threads
exist for the PR author to address but don't gate maintainer review.
Row F4 (`already marked ready, no regression`) gets the same
collaborator qualifier for consistency — otherwise a contributor
thread on a ready-labelled PR would falsely flag it as regressed.
Concrete impact on apache/airflow today: 6 PRs (SUCCESS, no conflicts,
0 collab threads, but 1-4 contributor-author threads) reached the
maintainer review queue via this rule rather than falling through.
**2. Author-primary templates — `<reviewer_logins>` placeholder +
explicit "resolve + ping" hand-back.**
When a comment's only addressee is the PR author (the three
author-primary templates: `request-author-confirmation`,
`reviewer-ping` author-primary, `review-nudge` author-primary), the
existing `<reviewers>` placeholder rendered as `@login` and produced
a notification for the reviewer — even though the next action is on
the author and the reviewer shouldn't be pinged.
Adds a sibling placeholder `<reviewer_logins>` that renders the same
logins as backtick-quoted code (no `@`), suitable for author-primary
templates. The reviewer is named for context without producing a
notification.
The three author-primary template bodies also pick up an explicit
hand-back instruction: when the author believes the feedback is
addressed, they mark the thread(s) resolved and `@`-mention the
reviewer themselves — the ping then comes from the author (who has
done the work) rather than from the bot (which only saw engagement).
Reviewer-re-review variants and `mark-ready-with-ping` keep using
`<reviewers>` (the `@`-form) because in those flows the reviewer IS
the next-action recipient.
The new "Reviewer-mention policy" section spells out the
placeholder / template mapping.
Tested against apache/airflow's adopter overrides for several weeks
before upstreaming. The placeholder addition is backward compatible —
adopters that don't update their overrides continue to use the
literal `<reviewers>` rendering.
---
.../pr-management-triage/classify-and-act.md | 6 +-
.../pr-management-triage/comment-templates.md | 67 ++++++++++++++++++----
2 files changed, 59 insertions(+), 14 deletions(-)
diff --git a/.claude/skills/pr-management-triage/classify-and-act.md
b/.claude/skills/pr-management-triage/classify-and-act.md
index ca06f9b..6efbda6 100644
--- a/.claude/skills/pr-management-triage/classify-and-act.md
+++ b/.claude/skills/pr-management-triage/classify-and-act.md
@@ -44,7 +44,7 @@ filter is skipped silently from the main triage flow.
| F1 | Author is collaborator/member/owner | `authorAssociation ∈ {OWNER,
MEMBER, COLLABORATOR}` (override: `authors:all` or `authors:collaborators`) |
| F2 | Author is a known bot | login is `dependabot`, `dependabot[bot]`,
`renovate[bot]`, `github-actions`, `github-actions[bot]`, or matches `*[bot]`.
Bot-authored **draft** PRs are handled separately by [`SKILL.md` Step
0.5](SKILL.md#step-05--promote-bot-authored-draft-prs) *before* this filter
runs; F2 then drops the same logins from the main triage flow regardless of
whether Step 0.5 promoted them. |
| F3 | Draft and not stale | `isDraft == true` and any activity within the
last 14 days. Stale-sweep classifications in
[`stale-sweeps.md`](stale-sweeps.md) may still pull the PR back in. |
-| F4 | Already marked ready, no regression | `labels` contains `ready for
maintainer review` AND CI green AND `mergeable != CONFLICTING` AND no
unresolved threads. **Regression bypasses this filter** — any of: CI red, new
conflict, or new unresolved thread whose triggering event (failing check
`startedAt`, conflict detection, thread `createdAt`) is *after* the label-add
timestamp. The typical case is a contributor pushing a rebase or fixup commit
to a ready-for-review PR that re-introduc [...]
+| F4 | Already marked ready, no regression | `labels` contains `ready for
maintainer review` AND CI green AND `mergeable != CONFLICTING` AND no
unresolved **collaborator** threads (same collaborator-author qualifier as rows
19/20 / [`unresolved_threads_only`](#unresolved_threads_only) —
contributor-author threads alone don't count as a regression for an
already-ready PR). **Regression bypasses this filter** — any of: CI red, new
conflict, or a new unresolved collaborator thread whose tri [...]
| F5a | Recent collaborator comment (author cooldown) | Most recent comment
from the **union of** general-issue comments (`comments(last:10)`) and
**review-thread comments** (`reviewThreads.nodes.comments`) is by a
`COLLABORATOR`/`MEMBER`/`OWNER`, `createdAt < 72h` ago, AND posted after
`commits(last:1).committedDate`. The review-thread leg is essential — a
maintainer asking a clarifying question in-thread is just as much an active
conversation as a top-level comment, and treating only t [...]
| F5b | Maintainer-to-maintainer ping unanswered | Most recent collaborator
comment from the **union** described in F5a above `@`-mentions one or more
logins other than the PR author AND none of those mentioned logins have posted
on the PR (general comments **or** review threads) or in `latestReviews` after
that comment. Team mentions (e.g. `@<upstream>-committers`) are conservatively
treated as F5b matches. |
| F6 | Maintainer co-drafted | `isDraft == true` AND any of: (a)
`latestReviews` has a node with `authorAssociation ∈ {OWNER, MEMBER,
COLLABORATOR}` AND `author.login ≠ <viewer>` AND `state ∈ {COMMENTED,
CHANGES_REQUESTED, APPROVED}` AND `submittedAt > commits(last:1).committedDate`
AND review body is non-empty (avoids the "review with only inline thread
comments and an empty top-level body" false positive — those are already
counted by row 14/15 unresolved-thread logic); (b) `comments(l [...]
@@ -93,8 +93,8 @@ Action verbs are defined in [`actions.md`](actions.md).
| 16 | No real CI ran (see [Real-CI guard](#real-ci-guard)) AND `mergeable !=
CONFLICTING` AND author NOT first-time | `deterministic_flag` | `rebase`
| No real CI checks triggered, branch mergeable — rebase to re-trigger |
| 17 | [`has_deterministic_signal`](#has_deterministic_signal) (fallback)
| `deterministic_flag` | `draft` |
Has quality issues — convert to draft with violations comment |
| 18 | `latestReviews` has CHANGES_REQUESTED AND author committed after AND
NOT [`follow_up_ping`](#follow_up_ping) | `stale_review` | `ping`
| Author pushed commits after CHANGES_REQUESTED from <reviewers> but
no follow-up — ping |
-| 19 | All of: `statusCheckRollup.state == SUCCESS`, `mergeable !=
CONFLICTING`, no unresolved threads, [Real-CI guard](#real-ci-guard) passes,
label `ready for maintainer review` already present | `passing` | `skip` |
Already marked ready for review |
-| 20 | All of: `statusCheckRollup.state == SUCCESS`, `mergeable !=
CONFLICTING`, no unresolved threads, [Real-CI guard](#real-ci-guard) passes |
`passing` | `mark-ready` | All checks green, no conflicts, no unresolved
threads — mark for deeper review |
+| 19 | All of: `statusCheckRollup.state == SUCCESS`, `mergeable !=
CONFLICTING`, no unresolved **collaborator** threads (see
[`unresolved_threads_only`](#unresolved_threads_only) for the
collaborator-author qualifier), [Real-CI guard](#real-ci-guard) passes, label
`ready for maintainer review` already present | `passing` | `skip` | Already
marked ready for review |
+| 20 | All of: `statusCheckRollup.state == SUCCESS`, `mergeable !=
CONFLICTING`, no unresolved **collaborator** threads (see
[`unresolved_threads_only`](#unresolved_threads_only) for the
collaborator-author qualifier), [Real-CI guard](#real-ci-guard) passes |
`passing` | `mark-ready` | All checks green, no conflicts, no unresolved
collaborator threads — mark for deeper review |
| 21 | Stale-sweep candidate (see [`stale-sweeps.md`](stale-sweeps.md)) AND no
row 1–20 matched in this session | `stale_draft` / `inactive_open` /
`stale_workflow_approval` | (per sweep) | (per sweep) |
| 22 | Data inconsistency: rollup `SUCCESS` with `failed_checks` non-empty, OR
rollup `FAILURE` with `failed_checks` empty (e.g. only CANCELLED contexts
visible, or rollup hasn't yet propagated the failing check-run). Evaluated
**before** rows 17, 19-20 — see [hard
rules](#hard-rules-cross-cutting-the-table) | n/a | `skip` | Data anomaly —
rollup not yet settled, retry next page |
diff --git a/.claude/skills/pr-management-triage/comment-templates.md
b/.claude/skills/pr-management-triage/comment-templates.md
index bbfca53..a6955e9 100644
--- a/.claude/skills/pr-management-triage/comment-templates.md
+++ b/.claude/skills/pr-management-triage/comment-templates.md
@@ -19,7 +19,19 @@ Placeholders:
- `<commits_behind>` — integer
- `<flagged_count>` — number of currently-flagged PRs by this
author (for the `close` template)
-- `<reviewers>` — space-separated `@login` mentions
+- `<reviewers>` — space-separated `@login` mentions. **Use in
+ reviewer-re-review variants and `mark-ready-with-ping` only**
+ — those templates address the reviewer as the next-action
+ recipient, so `@`-pinging them is appropriate.
+- `<reviewer_logins>` — comma-separated backtick-quoted logins
+ (e.g. `` `phanikumv` ``, `` `phanikumv`, `eladkal` ``) —
+ **no `@`-pings**. Use in author-primary templates
+ (`request-author-confirmation`, `reviewer-ping` author-primary,
+ `review-nudge` author-primary) where the only addressee is the
+ PR author. The reviewer is mentioned for context; an `@`-ping
+ would needlessly notify them when the next action is on the
+ author. See the [Reviewer-mention policy](#reviewer-mention-policy)
+ section below.
- `<days_since_triage>` — integer, for the stale-draft close
comment
@@ -38,6 +50,36 @@ anchor text breaks the re-triage skip logic.
---
+## Reviewer-mention policy
+
+When a comment's only addressee is the PR author (the
+`request-author-confirmation`, `reviewer-ping` author-primary,
+and `review-nudge` author-primary templates), the body references
+the reviewer **without** `@`-mentioning them. The default
+`<reviewers>` placeholder renders as `@login` and is appropriate
+when the reviewer is one of the message's addressees (e.g. the
+reviewer-re-review variants and `mark-ready-with-ping`). In
+author-primary templates we use a different placeholder,
+`<reviewer_logins>`, that renders the same logins **as
+backtick-quoted code** (e.g. `` `phanikumv` ``,
+`` `phanikumv`, `eladkal` ``) — recognisable as a GitHub handle
+without producing a notification.
+
+The policy: a reviewer is mentioned for context; an `@`-ping is
+only appropriate when the reviewer is the next person whose
+action we are asking for. Author-primary templates additionally
+tell the author what to do *when* they're ready (mark threads
+resolved + `@`-mention the reviewer themselves), so the ping
+happens — driven by the author, after they've engaged — rather
+than by the bot.
+
+| Placeholder | Renders as | Use in |
+|---|---|---|
+| `<reviewers>` | `@login [, @login ...]` | reviewer-re-review variants,
`mark-ready-with-ping` |
+| `<reviewer_logins>` | `` `login` [, `login` ...] `` (no `@`) |
`request-author-confirmation`, `reviewer-ping` (author-primary), `review-nudge`
(author-primary) |
+
+---
+
## AI-attribution footer
**Every contributor-facing template below ends with this
@@ -222,7 +264,7 @@ actually done the work yet.
### Default — author-primary nudge *(use unless the inspection below says
otherwise)*
```markdown
-@<author> — This PR has new commits since the last review requesting changes
from <reviewers>. Could you address the outstanding review comments and either
push a fix or reply in each thread explaining why the feedback doesn't apply?
Once the threads are resolved please mark the PR as "Ready for review" and
re-request review. Thanks!
+@<author> — This PR has new commits since the last review requesting changes
from <reviewer_logins>. Could you address the outstanding review comments and
either push a fix or reply in each thread explaining why the feedback doesn't
apply? When you believe the threads are resolved, please mark them as resolved
and ping the reviewer (<reviewer_logins>) — they'll either re-review or hand
the PR back to the queue. Thanks!
<ai_attribution_footer>
```
@@ -290,7 +332,7 @@ be resolved.
### Default — author-primary nudge *(use unless the inspection below says
otherwise)*
```markdown
-@<author> — There are <N> unresolved review thread(s) on this PR from
<reviewers>. Could you either push a fix or reply in each thread explaining why
the feedback doesn't apply? Once you believe the feedback is addressed, mark
the thread as resolved so the reviewer isn't re-pinged needlessly. Thanks!
+@<author> — There are <N> unresolved review thread(s) on this PR from
<reviewer_logins>. Could you either push a fix or reply in each thread
explaining why the feedback doesn't apply? When you believe the feedback is
addressed, please mark the threads as resolved and ping the reviewer
(<reviewer_logins>) for a final look. Thanks!
<ai_attribution_footer>
```
@@ -332,9 +374,9 @@ precondition searches for that exact text on subsequent
sweeps;
paraphrasing it breaks the gated flow.
```markdown
-@<author> — There are <N> unresolved review thread(s) on this PR from
<reviewers>, and you have engaged with each one (post-review commits and/or
in-thread replies). Could you confirm whether you believe the feedback is fully
addressed and the PR is ready for maintainer review confirmation?
+@<author> — There are <N> unresolved review thread(s) on this PR from
<reviewer_logins>, and you have engaged with each one (post-review commits
and/or in-thread replies). Could you confirm whether you believe the feedback
is fully addressed and the PR is ready for maintainer review confirmation?
-If yes, reply here (a short "yes / ready" is fine) and an <PROJECT> maintainer
will pick the PR up from the review queue on the next sweep.
+If yes, please mark the thread(s) as resolved and ping the reviewer
(<reviewer_logins>) for a final look. They will either label the PR ready for
maintainer review or follow up with additional feedback.
If you are still working on a thread, please reply with what is outstanding so
the threads stay unresolved on purpose.
@@ -344,12 +386,15 @@ If you are still working on a thread, please reply with
what is outstanding so t
Notes on the body:
- **`@<author>` is mentioned once at the top.** They are the
- only person whose input we need in this step.
-- **No `<reviewers>` `@`-mention.** Reviewers are reached
- through the `ready for maintainer review` queue once the
- author confirms — pinging them here would push a notification
- built on the engagement signal alone, which is the failure
- mode this two-sweep flow exists to avoid.
+ only person whose input we need *right now*.
+- **`<reviewer_logins>` (not `<reviewers>`) for the reviewer
+ reference.** Backtick-quoted, no `@`-ping. The reviewer is
+ mentioned for context — telling the author who left the
+ feedback — but pinging them in our message would build a
+ notification on the engagement signal alone, which is the
+ failure mode this two-sweep flow exists to avoid. The author
+ pings the reviewer themselves when they're ready, completing
+ the hand-back. See [Reviewer-mention policy](#reviewer-mention-policy).
- **The marker string `ready for maintainer review
confirmation`** appears verbatim in the first paragraph.
Do not edit the wording around it in a way that breaks the
