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 01d71dd feat(pr-management-triage): keep ready label during merit
discussion (#232)
01d71dd is described below
commit 01d71ddf5c2aaf29de03c9a32b80ff208ae55c63
Author: Jarek Potiuk <[email protected]>
AuthorDate: Tue May 19 23:48:09 2026 +0100
feat(pr-management-triage): keep ready label during merit discussion (#232)
Add a merit-discussion-in-flight exception to the
strip-ready-on-downgrade hard rule. When the PR has an
unresolved review thread whose first comment is from a
COLLABORATOR/MEMBER/OWNER, the `ready for maintainer
review` label is preserved on regression — `draft` actions
additionally skip the draft conversion, `close` actions
skip the close (comment + quality-violations label still
apply), `comment` actions just post without stripping.
The precondition is deliberately broad — any
maintainer-opened unresolved thread satisfies it,
regardless of body length or whether it was opened before
or after the label-add. Contributor-author threads alone
do not satisfy it.
Source: user-scope feedback memory
`feedback-ready-for-maintainer-review-label`. CI red and
a live design debate are orthogonal; a maintainer can
weigh in on design even when CI is red.
Files touched (skill `pr-management-triage`):
- classify-and-act.md — new `merit_discussion_thread_present`
precondition + exception block in the
`strip-ready-on-downgrade` hard rule.
- actions.md — Case A / Case B branches in `draft`,
`comment`, and `close` recipes; preview must quote the
maintainer-thread URLs that triggered the exception.
- rationale.md — new "Merit-discussion exception" section
explaining why the precondition is broad.
Out of scope: \`stale-sweeps.md\` Sweep 4 also strips the
ready label, but on a different signal (author silence
>= 7 days) — flagged for follow-up if wanted.
Generated-by: Claude Code (Opus 4.7)
---
.claude/skills/pr-management-triage/actions.md | 65 +++++++++++++++++--
.../pr-management-triage/classify-and-act.md | 72 +++++++++++++++++++++-
.claude/skills/pr-management-triage/rationale.md | 44 +++++++++++++
3 files changed, 174 insertions(+), 7 deletions(-)
diff --git a/.claude/skills/pr-management-triage/actions.md
b/.claude/skills/pr-management-triage/actions.md
index 9db69a9..9a1fc76 100644
--- a/.claude/skills/pr-management-triage/actions.md
+++ b/.claude/skills/pr-management-triage/actions.md
@@ -50,9 +50,16 @@ on a still-open PR is a worse state than no comment at all.
The PR bypassed F4 because of post-label regression (rebase /
push re-introduced a deterministic failure — see
[`strip-ready-on-downgrade`](classify-and-act.md#hard-rules-cross-cutting-the-table)).
-Strip the label as the **first** mutation, before converting
-to draft, so the queue position is corrected even if a later
-step fails:
+
+**Branch on the merit-discussion exception.** Before mutating,
+evaluate
+[`merit_discussion_thread_present`](classify-and-act.md#merit_discussion_thread_present)
+on the PR.
+
+**Case A — no merit discussion present** (the strip-and-draft
+default). Strip the label as the **first** mutation, before
+converting to draft, so the queue position is corrected even
+if a later step fails:
```bash
# 0. Remove the now-stale ready-for-review label (idempotent —
@@ -75,6 +82,26 @@ manually), but stranding the PR in a half-state would be
worse. The maintainer-facing preview should note when step 0
will run so the proposal is honest about both state changes.
+**Case B — merit discussion present** (per the exception in
+[`strip-ready-on-downgrade`](classify-and-act.md#hard-rules-cross-cutting-the-table)).
+Skip step 0 (label stays) and step 1 (PR stays out of draft).
+Post only the violations comment:
+
+```bash
+# 1. Post the violations comment (label stays; PR stays open).
+gh pr comment <N> --repo <repo> --body-file /tmp/pr-<N>-draft-body.md
+```
+
+The maintainer-facing preview MUST surface that the merit
+discussion was detected and that the action is being
+de-escalated from `draft` to `comment-only` for this reason
+— include the URLs of the maintainer-opened unresolved review
+thread(s) that triggered the exception so the maintainer can
+sanity-check the call. The violations comment body is
+unchanged from Case A; it informs the author that mechanical
+issues remain even though the discussion is what's keeping
+the label on.
+
### If the PR is already a draft
Skip the `gh pr ready --undo` step. Post only the comment. The
@@ -116,10 +143,14 @@ the people it's for.
When the upstream classification is `deterministic_flag` and the
PR carries the label (regression bypass of F4 — see
[`strip-ready-on-downgrade`](classify-and-act.md#hard-rules-cross-cutting-the-table)),
-strip the label **before** posting the comment:
+strip the label **before** posting the comment — **unless**
+[`merit_discussion_thread_present`](classify-and-act.md#merit_discussion_thread_present)
+holds, in which case the label stays and only the comment is
+posted.
```bash
# 0. Remove the now-stale ready-for-review label.
+# SKIP this step when merit_discussion_thread_present holds.
gh pr edit <N> --repo <repo> --remove-label "ready for maintainer review"
# 1. Post the violations comment
@@ -136,6 +167,11 @@ or static-check-only failures). `stale_review` and explicit
signals and the ready-for-review queue position is still valid
information for the reviewer.
+When the merit-discussion exception applies, the
+maintainer-facing preview MUST surface that step 0 is being
+skipped and quote the URL(s) of the maintainer-opened
+unresolved review thread(s) that triggered the exception.
+
---
## `close` — close with comment and quality-violations label
@@ -166,6 +202,27 @@ inside a `close` group, the maintainer confirms each PR
individually — a wrongly-closed PR is the hardest mistake to
recover from.
+### If the PR carries `ready for maintainer review` and a merit discussion is
in flight
+
+When the PR carries `ready for maintainer review` AND
+[`merit_discussion_thread_present`](classify-and-act.md#merit_discussion_thread_present)
+holds, the
+[`strip-ready-on-downgrade`](classify-and-act.md#hard-rules-cross-cutting-the-table)
+exception applies: **skip step 2** (do not close the PR) and
+**do not strip the ready-for-maintainer-review label**. Steps
+1 and 3 still run — the close comment surfaces the
+queue-pressure reasoning, and the quality-violations label
+records that the PR was flagged. The PR remains open with
+both labels, surfaced for human review.
+
+The maintainer-facing preview MUST surface that step 2 is
+being skipped and quote the URL(s) of the maintainer-opened
+unresolved review thread(s) that triggered the exception.
+Closing a PR with an active maintainer review discussion is
+strictly more destructive than the queue-pressure problem
+`close` exists to solve — a human maintainer must make that
+call, not the skill.
+
---
## `mark-ready` — add `ready for maintainer review` label
diff --git a/.claude/skills/pr-management-triage/classify-and-act.md
b/.claude/skills/pr-management-triage/classify-and-act.md
index 6efbda6..7d5df4c 100644
--- a/.claude/skills/pr-management-triage/classify-and-act.md
+++ b/.claude/skills/pr-management-triage/classify-and-act.md
@@ -142,9 +142,47 @@ Action verbs are defined in [`actions.md`](actions.md).
NOT strip the label: those classify the regression as
transient (flaky CI, missing base merge, reviewer hasn't
responded) and the label is still informative if the
- follow-up succeeds. Implementation: see
-
[`actions.md#draft`](actions.md#draft--convert-to-draft-and-post-violations-comment)
- and
[`actions.md#comment`](actions.md#comment--post-violations--stale-review--ping-comment).
+ follow-up succeeds.
+
+ **Exception — merit-discussion-in-flight.** If
+ [`merit_discussion_thread_present`](#merit_discussion_thread_present)
+ holds on the regressed PR, the strip-ready-on-downgrade
+ rule does **not** fire. Additionally:
+
+ - A `draft` action skips the `gh pr ready --undo` step
+ (the PR stays out of draft). The violations comment is
+ still posted so mechanical issues remain surfaced for
+ the author. The action effectively degrades to a
+ `comment` action that preserves the label.
+ - A `close` action skips the close step (the PR stays
+ open). The comment is still posted; the
+ quality-violations label is still applied. Closing a PR
+ with an active maintainer review discussion is more
+ destructive than the queue-pressure problem `close`
+ exists to solve.
+ - A `comment` action posts the violations comment but
+ does not strip the label.
+
+ Rationale: the `ready for maintainer review` label exists
+ to attract senior eyes, and an unresolved maintainer review
+ thread is exactly the moment those eyes are most valuable.
+ Stripping the label or pushing the PR back to draft
+ mid-discussion makes it disappear from the maintainer queue
+ at the worst possible time. CI red / lint failures / merge
+ conflicts and a live design debate are orthogonal: a
+ maintainer can weigh in on design even when CI is red. The
+ precondition is deliberately broad — contributor-author
+ threads alone do not satisfy it (those are not the merit
+ signal the label defers to), but any maintainer-opened
+ unresolved thread does, regardless of body length or when
+ it was opened relative to the label-add. Source: user-scope
+ feedback memory
+ `feedback-ready-for-maintainer-review-label`.
+
+ Implementation: see
+
[`actions.md#draft`](actions.md#draft--convert-to-draft-and-post-violations-comment),
+
[`actions.md#comment`](actions.md#comment--post-violations--stale-review--ping-comment),
+ and
[`actions.md#close`](actions.md#close--close-with-comment-and-quality-violations-label).
---
@@ -241,6 +279,34 @@ but only `failed_checks` feeds the decision table.
fired is unresolved threads (`statusCheckRollup.state` is
`SUCCESS`, `mergeable != CONFLICTING`).
+### `merit_discussion_thread_present`
+
+True when the PR has at least one unresolved review thread
+whose first comment is from a `COLLABORATOR`/`MEMBER`/`OWNER`
+(the same collaborator-author qualifier as
+[`unresolved_threads_only`](#unresolved_threads_only)).
+
+This is the "active maintainer review discussion" signal. No
+timing qualifier is applied — a substantive design / approach
+/ scope / correctness discussion can have started either
+before or after the `ready for maintainer review` label was
+added, and in either case the label must not be stripped
+while the discussion is in flight. The precondition
+deliberately does not filter by body length or thread
+content: an explicit maintainer act of opening a review
+thread is treated as substantive engagement on its own.
+Contributor-author unresolved threads do NOT satisfy this
+precondition (mirrors the
+[`unresolved_threads_only`](#unresolved_threads_only)
+collaborator qualifier — contributor-to-contributor side
+chatter is not a merit discussion the label should defer to).
+
+Source: user-scope feedback memory
+`feedback-ready-for-maintainer-review-label`. See the
+[`strip-ready-on-downgrade`](#hard-rules-cross-cutting-the-table)
+hard rule for how this precondition gates the label-strip,
+draft-conversion, and close behavior on a regressed PR.
+
### `unresolved_threads_only_likely_addressed`
All of:
diff --git a/.claude/skills/pr-management-triage/rationale.md
b/.claude/skills/pr-management-triage/rationale.md
index ff1962d..f0b7f43 100644
--- a/.claude/skills/pr-management-triage/rationale.md
+++ b/.claude/skills/pr-management-triage/rationale.md
@@ -573,6 +573,50 @@ draft is an overreach.
---
+## Merit-discussion exception to `strip-ready-on-downgrade`
+
+The `strip-ready-on-downgrade` hard rule
+(see
[`classify-and-act.md`](classify-and-act.md#hard-rules-cross-cutting-the-table))
+otherwise strips `ready for maintainer review` whenever a
+regressed PR matches a `deterministic_flag` row with action
+`draft` / `comment` / `close`. The
+[`merit_discussion_thread_present`](classify-and-act.md#merit_discussion_thread_present)
+exception suspends that strip — and additionally suspends the
+draft conversion and the close — when an unresolved
+maintainer-opened review thread is present on the PR.
+
+Why this matters:
+
+- The `ready for maintainer review` label exists to attract
+ senior eyes. An unresolved maintainer review thread is the
+ moment senior eyes are most valuable. Stripping the label
+ or pushing the PR back to draft mid-discussion makes the
+ PR disappear from the maintainer queue exactly when it
+ shouldn't.
+- CI red / lint failures / merge conflicts and a live design
+ debate are orthogonal axes. A maintainer can usefully weigh
+ in on the design discussion even when CI is red — the
+ mechanical blockers belong to the author, the design
+ question belongs to the maintainers.
+- The exception's precondition is deliberately broad — any
+ maintainer-opened unresolved review thread counts,
+ regardless of body length or when it was opened relative
+ to the label-add. A narrower "substantive content"
+ heuristic would mis-classify short-but-substantive prompts
+ ("is this really the right layer for this change?") as
+ trivial and strip the label anyway. Erring toward keeping
+ the label is the safer asymmetry: a stale-but-kept label
+ costs a maintainer a glance; a stripped label mid-discussion
+ costs the discussion its audience.
+- Contributor-author unresolved threads do NOT satisfy the
+ precondition. The label defers to maintainer judgment, not
+ contributor-to-contributor side chatter.
+
+Originating user-scope feedback memory:
+`feedback-ready-for-maintainer-review-label`.
+
+---
+
## Group-level overrides
The interaction loop lets the maintainer override the suggested
