From 7e09c55760ab599bc9dcb04f1e196bd88b37a07f Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Tue, 19 May 2026 23:40:03 +0100 Subject: [PATCH] feat(pr-management-triage): keep ready label during merit discussion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../skills/pr-management-triage/actions.md | 65 +++++++++++++++-- .../pr-management-triage/classify-and-act.md | 72 ++++++++++++++++++- .../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 9db69a96..9a1fc76e 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 --repo --body-file /tmp/pr--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 --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 6efbda61..7d5df4c6 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 ff1962d9..f0b7f43c 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