Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 17 additions & 6 deletions skills/pr-management-triage/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -294,10 +294,10 @@ variant — same calibration, worded for a description edit. See
and [`comment-templates.md#body-fold-rendering`](comment-templates.md#body-fold-rendering).

**Golden rule 9 — never talk over an active maintainer
conversation.** When a maintainer has commented on the PR
recently, the skill steps back. Two specific cases, both
conversation.** When a human conversation needs the next move,
the skill steps back. Three specific cases, all
enforced as pre-classification filters in
[`classify-and-act.md#pre-filters`](classify-and-act.md) (rows F5a, F5b):
[`classify-and-act.md#pre-filters`](classify-and-act.md) (rows F5a, F5b, F5c):

- **Author-response cooldown (≥ 72 hours).** If the most recent
comment by a `COLLABORATOR`/`MEMBER`/`OWNER` was posted after
Expand All @@ -312,6 +312,16 @@ enforced as pre-classification filters in
author should work on comments" auto-draft de-focuses the
thread away from the input the original commenter was asking
for.
- **Author question to a maintainer (ball in our court).** The
inverse of the maintainer-to-maintainer case: if the most
recent human comment is by the **PR author** and `@`-mentions a
maintainer (or the committers team) with no maintainer reply
after it, the author is waiting on *us*. Skip the author-facing
flow — never ping the author, request readiness confirmation,
convert to draft, or close it for "silence". The next move is a
maintainer answering; the PR belongs in the maintainers' court.
This is the case that closed a real PR after the triage process
missed an open question to the team.

These filters override every deterministic flag (failing CI,
conflicts, unresolved threads). The cost of a missed auto-action
Expand Down Expand Up @@ -553,11 +563,12 @@ pulls a PR out of a group.
Run **every PR fetched in Step 1** through
[`classify-and-act.md`](classify-and-act.md), once:

1. Apply the [pre-filters](classify-and-act.md#pre-filters) (F1–F5b)
1. Apply the [pre-filters](classify-and-act.md#pre-filters) (F1–F5c)
to drop collaborator PRs, bot accounts, fresh drafts,
already-marked-ready PRs without regression, and PRs with an
active maintainer conversation (72-hour author cooldown or an
unanswered maintainer-to-maintainer ping).
active maintainer conversation (72-hour author cooldown, an
unanswered maintainer-to-maintainer ping, or an unanswered
author question to a maintainer — ball in our court).
2. Evaluate the [decision table](classify-and-act.md#decision-table)
top-to-bottom. The first matching row yields the
`(classification, action, reason)` tuple for that PR.
Expand Down
48 changes: 45 additions & 3 deletions skills/pr-management-triage/classify-and-act.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,16 @@ filter is skipped silently from the main triage flow.
| 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 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-introduces deterministic failures, OR a maintainer leaving a new review thread post-label-add. PRs bypassing F4 fall through to the decision table normally; the cross-cutting [`strip-ready-on-downgrade` hard rule](#hard-rules-cross-cutting-the-table) ensures the label comes off if a `deterministic_flag` row fires. |
| 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 the latter routes the PR to `ping` / `request-author-confirmation` while the maintainer is still mid-sentence. |
| F5b | Maintainer-to-maintainer ping unanswered | **Walk the most recent 5 collaborator comments** from the **union** described in F5a above (newest-first). For each, if it `@`-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, F5b fires. The 5-comment window catches the case where maintainer A pings maintainer B, then a different maintainer C (often the viewer) posts a quality-criteria comment on the PR — the more-recent quality comment is not itself a ping, but the older A→B ping is still unanswered and the PR is still mid-conversation between maintainers. Without the deep scan F5b only inspects the most recent collaborator comment and the older ping slides past unfiltered. The 5-comment cap is the same one driving `comments(last:10)` / `reviewThreads.comments(first:5)` — bounded scan, no extra GraphQL. Team mentions (e.g. `@<upstream>-committers`) are conservatively treated as F5b matches. |
| F5c | Author question to a maintainer unanswered (ball in maintainers' court) | [`author_question_to_maintainer_unanswered`](#author_question_to_maintainer_unanswered) holds — the most recent **human** comment on the PR is by the **PR author** and `@`-mentions a maintainer (or the committers team) with no maintainer reply after it. This is the **inverse of F5b**: the author is waiting on *maintainer* input, so the next move is a maintainer's. Skip the main triage flow — do **not** hand the PR back to the author (`ping`, `request-author-confirmation`), convert it to draft, or close it for author silence. The PR is in the maintainers' court; surfacing it for a human answer is a maintainer move outside triage's mutation scope. Team mentions are conservatively treated as matches (same as F5b). |
| 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(last:10)` has a node with `authorAssociation ∈ {OWNER, MEMBER, COLLABORATOR}` AND `author.login ≠ <viewer>` AND `length(bodyText) ≥ 80` AND `createdAt > commits(last:1).committedDate`. Trivial signals (emoji-only, `+1`, `lgtm`, pure `@team` pings without prose) do not count — those are already covered by F5a/F5b or are below the substantive-engagement threshold. **Stale-sweep classifications in [`stale-sweeps.md`](stale-sweeps.md) may still pull the PR back in** — F6 only suppresses duplicate-proposal rows from the decision table, not eventual-resurfacing on a different action. |

F5a, F5b, and F6 override every signal in the decision table —
F5a, F5b, F5c, and F6 override every signal in the decision table —
they are not weighed against conflicts, failing CI, or unresolved
threads. See
threads. (F5c in particular overrides a merge conflict or red CI:
when the author has asked us a question, auto-pinging them to "fix
CI" while their question sits unanswered is exactly the talk-over
F5a/F5b guard against — the maintainer answers, and asks for the
CI fix themselves if needed.) See
[`rationale.md#pre-filter-5-active-maintainer-conversation`](rationale.md#pre-filter-5-active-maintainer-conversation)
and
[`rationale.md#pre-filter-6-maintainer-co-drafted`](rationale.md#pre-filter-6-maintainer-co-drafted)
Expand Down Expand Up @@ -227,6 +232,43 @@ decisions (a Sweep-4 candidate, an F5a/F5b comment that would suppress a
ping), not for every PR in the batch, so it adds no per-PR cost to the
main sweep.

### `author_question_to_maintainer_unanswered`

All of:

- The most recent **human** comment on the PR — taken from the
**union** of issue-level comments (`comments(last:10)`) and
review-thread comments (`reviewThreads.nodes.comments(first:5)`),
newest by `createdAt`, ignoring `*[bot]` authors — is by the
**PR author** (`author.login == pr.author.login`). A later
bot comment does **not** reset the match.
- That comment `@`-mentions at least one **maintainer** (resolved
per [Maintainer activity](#maintainer-activity) — a
`committers_team` member or an account with
`write`/`maintain`/`admin`, **not** `authorAssociation` alone),
**or** `@`-mentions the committers team.
- No maintainer has posted a comment (issue-level or review-thread)
or a review (`latestReviews`) with `createdAt` / `submittedAt`
after that author comment.

This is the **inverse of [F5b](#pre-filters)**: F5b is a maintainer
waiting on another maintainer; this is the **author waiting on a
maintainer**. In both, the next move is a human maintainer's, so
the ball is in the maintainers' court and the triage skill must not
hand the PR back to the author, convert it to draft, or close it
for "author silence". It is the precondition behind pre-filter
[F5c](#pre-filters) and the maintainer-court guard in
[`stale-sweeps.md`](stale-sweeps.md).

Maintainer resolution is the same load-bearing live check
[F5b / Sweep 4](#maintainer-activity) use — confirm a
`COLLABORATOR` mention's committer status via the `permission` /
team-membership API before relying on it, for the small set of PRs
where this precondition is the deciding factor (a Sweep-4 candidate,
or a PR F5c would otherwise skip). Team mentions
(e.g. `@<upstream>-committers`) are conservatively treated as
matches — same calibration as F5b.

### `has_deterministic_signal`

At least one of:
Expand Down Expand Up @@ -712,7 +754,7 @@ applies — rows do not get to reach back for more data.

| Decision rows / preconditions | Required fields |
|---|---|
| F5a, F5b, F6, grace periods | `comments(last:10).nodes.{author.login,authorAssociation,bodyText,createdAt}`, `reviewThreads.nodes.comments(first:5).nodes.{author.login,authorAssociation,bodyText,createdAt}`, `latestReviews.nodes.{state,author.login,authorAssociation,submittedAt}`, `commits(last:1).nodes.commit.committedDate`, viewer login |
| F5a, F5b, F5c, F6, grace periods | `comments(last:10).nodes.{author.login,authorAssociation,bodyText,createdAt}`, `reviewThreads.nodes.comments(first:5).nodes.{author.login,authorAssociation,bodyText,createdAt}`, `latestReviews.nodes.{state,author.login,authorAssociation,submittedAt}`, `commits(last:1).nodes.commit.committedDate`, viewer login |
| Row 1 + Real-CI guard | `statusCheckRollup.state`, `statusCheckRollup.contexts`, `authorAssociation`, `head_sha` (REST `action_required` index keyed by `head_sha`) |
| `copilot_review_stale` (row 2) | `reviewThreads.nodes.{isResolved,comments.nodes.{author.login,createdAt,url}}`, `comments(last:10).nodes.{author.login,createdAt}` |
| `has_deterministic_signal`, `ci_failures_only`, `unresolved_threads_only`, `unresolved_threads_only_likely_addressed` (rows 8–17) | `mergeable`, `statusCheckRollup.{state,contexts}`, `reviewThreads.nodes.{isResolved,comments(first:5).nodes.{author.login,authorAssociation,createdAt}}`, `updatedAt`, `comments(last:10).nodes.{author.login,authorAssociation,createdAt}`, `commits(last:1).nodes.commit.committedDate`, `author.login` |
Expand Down
5 changes: 3 additions & 2 deletions skills/pr-management-triage/fetch-and-batch.md
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,9 @@ pick action). Nothing here is speculative:
responded after triage" detection, and the
active-maintainer-conversation pre-filter
(recent collaborator comment + maintainer-to-maintainer
`@`-ping detection — see
[`classify-and-act.md#pre-filters`](classify-and-act.md), F5a/F5b)
`@`-ping detection + author-to-maintainer `@`-ping detection —
see [`classify-and-act.md#pre-filters`](classify-and-act.md),
F5a/F5b/F5c)
— which is why the comment node carries `authorAssociation`
and `bodyText` in addition to author login
- `baseRef.target.history.totalCount` → commits-behind anchor
Expand Down
48 changes: 44 additions & 4 deletions skills/pr-management-triage/rationale.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,11 @@ information — fix it there, do not re-derive from prose here.

## Pre-filter 5 (active maintainer conversation)

F5a (author-response cooldown) and F5b (maintainer-to-maintainer
ping) override every signal in the decision table. Two cases,
same underlying principle: do not let the triage skill talk over
a human conversation already in progress.
F5a (author-response cooldown), F5b (maintainer-to-maintainer
ping), and F5c (author question to a maintainer) override every
signal in the decision table. Three cases, same underlying
principle: do not let the triage skill talk over — or pre-empt —
a human conversation that needs a human's next move.

### F5a — 72-hour cooldown after a collaborator comment

Expand Down Expand Up @@ -73,6 +74,45 @@ in the batch query, and the false-positive cost (skipping a PR
that should have been actioned) is much lower than the false-
negative cost (talking over a real maintainer call-out).

### F5c — author question to a maintainer unanswered

The mirror image of F5b. F5b is *maintainer → maintainer*; F5c is
*author → maintainer*. When the author's most recent comment pings
a maintainer (*"@potiuk I rebased and answered your point — could
you take another look?"*, or any open question directed at the
team) and no maintainer has replied since, the PR is waiting on
**us**, not on the author. The ball is in the maintainers' court.

Without F5c the classifier reads "last comment by author, then
silence" the same way it reads an abandoned PR, and routes it to an
author-facing action — ping the author, request readiness
confirmation, convert to draft, or (via the stale sweeps) close it
for inactivity. Every one of those is wrong here: the contributor
already did their part and asked us a question; handing the PR back
to them, or closing it, tells a contributor who was waiting politely
that the project dropped their question on the floor. That is the
single most corrosive contributor experience the skill can produce —
and it is exactly what happened to a real PR that the triage process
closed while an open question to the team sat unanswered, later
reopened by hand with an apology.

The override is deliberately as strong as F5a/F5b — it beats a merge
conflict and red CI. If the author asked a question *and* CI is red,
auto-pinging "please fix CI" while ignoring their question is the
talk-over we are trying to prevent. The maintainer answers, and if a
CI fix is also needed they say so themselves, in their own voice.

Maintainer status is resolved the same load-bearing way as F5b and
Sweep 4 — a `COLLABORATOR` mention is confirmed against the
committers team / repo-permission API before it is trusted, so a
ping at a read-only collaborator does not falsely park the PR in our
court. Team mentions are conservatively treated as matches, same as
F5b. The detection is `@`-mention-based on purpose: "did the author
ask a question?" is not cheaply decidable, but "did the author ping
a maintainer with no reply since?" is, and it is the deterministic
core of the case. A maintainer reading the surfaced PR makes the
final call on whether an answer is owed.

---

## Pre-filter 6 (maintainer co-drafted)
Expand Down
39 changes: 38 additions & 1 deletion skills/pr-management-triage/stale-sweeps.md
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,34 @@ cases (a PR updated mid-session) don't shift.

---

## Maintainer-court guard (applies to every sweep)

A PR satisfying
[`author_question_to_maintainer_unanswered`](classify-and-act.md#author_question_to_maintainer_unanswered)
— the author's most recent comment `@`-mentions a maintainer (or
the committers team) and no maintainer has replied since — is in
the **maintainers' court**: the next move is a maintainer
answering, not anything the author owes. Every sweep below honours
it:

- **Sweeps 1–3** (close / convert-to-draft for staleness) **skip
it.** A PR is not "abandoned by the author" while the author is
waiting on *us*; closing or drafting it for inactivity punishes
the contributor for maintainer silence. This is the exact
failure that closed a real PR after the triage process missed an
open question to the team.
- **Sweep 4** keeps the `ready for maintainer review` label (see
[Step B](#step-b--court-disposition)) — the label *means* "ball
in the maintainers' court", which is precisely true here.

Inactivity timers (`updated_at`-based) do **not** override this
guard; only a maintainer reply — which flips the PR out of the
precondition — does. The guard is the stale-sweep counterpart of
pre-filter [F5c](classify-and-act.md#pre-filters), which removes
the same PRs from the *interactive* flow.

---

## Sweep 1 — Stale drafts

Two sub-cases, both resulting in `close`:
Expand Down Expand Up @@ -305,7 +333,15 @@ decision table already draws that line (rows 10–13 `rerun` vs 12/12b/17
skill uses for the same reason — observed: a batch mergeability gate
misjudged ~87% of a real `ready` queue.

Then run the PR through the live decision table
**Check the maintainer-court guard first.** If
[`author_question_to_maintainer_unanswered`](classify-and-act.md#author_question_to_maintainer_unanswered)
holds (pre-filter [F5c](classify-and-act.md#pre-filters) would
have skipped it in the interactive flow), the PR is maintainer-court
regardless of branch state — keep the label and stop. Resolve the
mentioned maintainer's committer status live (per the section above)
before relying on this, since it is the deciding signal.

Otherwise run the PR through the live decision table
([`classify-and-act.md`](classify-and-act.md)) to get its current
`(classification, action)`, resolving "maintainer" per the section
above. Step B reads the court off that result.
Expand All @@ -316,6 +352,7 @@ Read the court off the Step-A `(classification, action)`:

| Next move (decision-table action / state) | Court | Sweep-4 action |
|---|---|---|
| [`author_question_to_maintainer_unanswered`](classify-and-act.md#author_question_to_maintainer_unanswered) — author asked a maintainer, unanswered | maintainer (respond) | **keep label** — the author is waiting on us; never strip |
| `approve-workflow` | maintainer | **keep label**; perform the approval |
| `rerun` — flaky / systemic CI (rows 10/11/13) | maintainer | **keep label**; perform the rerun |
| `rebase` — branch behind, mergeable (row 16) | maintainer | **keep label**; perform the branch update |
Expand Down