diff --git a/skills/pr-management-triage/SKILL.md b/skills/pr-management-triage/SKILL.md index 28b3b76b..05c059b2 100644 --- a/skills/pr-management-triage/SKILL.md +++ b/skills/pr-management-triage/SKILL.md @@ -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 @@ -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 @@ -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. diff --git a/skills/pr-management-triage/classify-and-act.md b/skills/pr-management-triage/classify-and-act.md index 855c0370..47a767a0 100644 --- a/skills/pr-management-triage/classify-and-act.md +++ b/skills/pr-management-triage/classify-and-act.md @@ -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. `@-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 ≠ ` 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 ≠ ` 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) @@ -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. `@-committers`) are conservatively treated as +matches — same calibration as F5b. + ### `has_deterministic_signal` At least one of: @@ -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` | diff --git a/skills/pr-management-triage/fetch-and-batch.md b/skills/pr-management-triage/fetch-and-batch.md index ac89ee3a..82240640 100644 --- a/skills/pr-management-triage/fetch-and-batch.md +++ b/skills/pr-management-triage/fetch-and-batch.md @@ -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 diff --git a/skills/pr-management-triage/rationale.md b/skills/pr-management-triage/rationale.md index ce581db8..1bb45a26 100644 --- a/skills/pr-management-triage/rationale.md +++ b/skills/pr-management-triage/rationale.md @@ -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 @@ -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) diff --git a/skills/pr-management-triage/stale-sweeps.md b/skills/pr-management-triage/stale-sweeps.md index dfea696f..435cf821 100644 --- a/skills/pr-management-triage/stale-sweeps.md +++ b/skills/pr-management-triage/stale-sweeps.md @@ -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`: @@ -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. @@ -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 |