From e1a9f323636195517417183fb486984e5d7abc35 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Mon, 20 Jul 2026 16:42:13 +0200 Subject: [PATCH 1/2] fix(pr-management-triage): stop misclassifying PRs on truncated and unsettled GitHub data MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects surfaced by a full 333-PR triage sweep, each of which produced wrong contributor-facing outcomes. 1. `statusCheckRollup.contexts` is a paginated connection whose page is a silent prefix. On a repo running 100+ check-runs per PR the derived failed-check list was truncated with no signal, so rows 10-13 and the Real-CI guard read an incomplete list: one PR showed 1 of 16 real failures and was routed to `comment` instead of `draft`; another showed 2 of 4 and was routed to `rerun`, treating a failure reproducing on all four DB backends as a flake. Raise the page to 100 and require the list be re-derived from the paginated check-runs REST API before any row reads it. `rollup.state` stays authoritative; only the derived list was ever unsafe. 2. `mergeable` is computed lazily and reports UNKNOWN until GitHub settles it. Rows 19/20 were written as `!= CONFLICTING`, which is true for UNKNOWN, so unsettled PRs classified as `passing`. Require `== MERGEABLE` explicitly and add a `mergeable_state` guard to the `mark-ready` recipe, folded into the PR fetch it already makes. On the observed sweep 11 of 39 mark-ready candidates reported UNKNOWN at fetch time and `dirty` at mutation time — all genuinely conflicting. 3. Stale-sweep 1a tested for author *comments* only, while its action is `close`. Contributors who answer review feedback by pushing code read as silent: one PR with author pushes 5, 12 and 25 days after the triage comment matched the trigger. Key it on the existing `last_author_activity_at` input, which already folds in pushes and review-thread replies. Generated-by: Claude Opus 4.8 (1M context) --- skills/pr-management-triage/actions.md | 26 ++++++- .../pr-management-triage/classify-and-act.md | 31 +++++++- .../pr-management-triage/fetch-and-batch.md | 75 ++++++++++++++++++- skills/pr-management-triage/stale-sweeps.md | 27 ++++++- 4 files changed, 150 insertions(+), 9 deletions(-) diff --git a/skills/pr-management-triage/actions.md b/skills/pr-management-triage/actions.md index 531aa0b44..11d33f7e6 100644 --- a/skills/pr-management-triage/actions.md +++ b/skills/pr-management-triage/actions.md @@ -362,7 +362,10 @@ executed. # with `conclusion: "action_required"`. The query parameter # `?status=action_required` matches no runs and would silently # return an empty result — post-filter on `conclusion` instead. -head_sha=$(gh api "repos///pulls/" --jq '.head.sha') +# One fetch covers both guards. +read -r head_sha merge_state <<<"$(gh api "repos///pulls/" \ + --jq '"\(.head.sha) \(.mergeable_state)"')" + pending=$(gh api "repos///actions/runs?head_sha=${head_sha}&per_page=20" \ --jq '[.workflow_runs[] | select(.conclusion == "action_required")] | length') if [ "$pending" -gt 0 ]; then @@ -371,10 +374,29 @@ if [ "$pending" -gt 0 ]; then exit 2 fi -# Guard passed — apply the label. +# Mergeability guard — GraphQL `mergeable` is computed lazily and +# reports UNKNOWN until a background job settles it, so a PR can +# classify as `passing` and be conflicting by the time we mutate. +# `mergeable_state == dirty` is the REST spelling of CONFLICTING. +if [ "$merge_state" = "dirty" ]; then + echo "refuse mark-ready: is conflicting — route to draft instead" >&2 + exit 2 +fi +if [ "$merge_state" = "unknown" ]; then + echo "refuse mark-ready: mergeability not yet computed — retry next sweep" >&2 + exit 2 +fi + +# Guards passed — apply the label. gh pr edit --repo --add-label "ready for maintainer review" ``` +When the mergeability guard refuses with `dirty`, the PR belongs +to row 9 (`mergeable == CONFLICTING` → `draft`) — route it there +rather than dropping it. On a full sweep of a large `` +this guard refused **11 of 39** `mark-ready` candidates, every one +genuinely conflicting despite reporting `UNKNOWN` at fetch time. + When the guard refuses, the implementation should **reclassify the PR as `pending_workflow_approval`** (see [`classify-and-act.md#decision-table`](classify-and-act.md), row 1) and diff --git a/skills/pr-management-triage/classify-and-act.md b/skills/pr-management-triage/classify-and-act.md index 6a611c7ca..22957659b 100644 --- a/skills/pr-management-triage/classify-and-act.md +++ b/skills/pr-management-triage/classify-and-act.md @@ -100,8 +100,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 but no follow-up — ping | -| 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 | +| 19 | All of: `statusCheckRollup.state == SUCCESS`, `mergeable == MERGEABLE` (**not** merely `!= CONFLICTING` — see [hard rules](#hard-rules-cross-cutting-the-table)), 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 == MERGEABLE` (**not** merely `!= CONFLICTING` — see [hard rules](#hard-rules-cross-cutting-the-table)), 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 | @@ -116,6 +116,33 @@ Action verbs are defined in [`actions.md`](actions.md). [`mark-ready` action](actions.md#mark-ready--add-ready-for-maintainer-review-label) re-checks the REST `action_required` index immediately before mutating (Golden rule 1b in [`SKILL.md`](SKILL.md)). +- **`mergeable == UNKNOWN` is not "no conflict".** GitHub + computes mergeability lazily: the first query after a base-branch + move returns `UNKNOWN` while a background job runs. Written as + `mergeable != CONFLICTING`, rows 19/20 evaluate **true** for + `UNKNOWN` — so an unsettled PR reads as green and earns + `ready for maintainer review`. + Treat `UNKNOWN` as *undetermined*, never as *mergeable*: + - Rows 19/20 require `mergeable == MERGEABLE` explicitly. A PR + with `UNKNOWN` falls through to `skip` with reason + *"mergeability not yet computed — retry next sweep"*; GitHub + settles it within seconds and the next sweep classifies it + properly. + - The [`mark-ready` action](actions.md#mark-ready--add-ready-for-maintainer-review-label) + re-reads `mergeable_state` from the REST PR object immediately + before applying the label and refuses on `dirty`, in the same + pre-mutation block as the `action_required` check. + + F4 keeps the looser `!= CONFLICTING` deliberately: it only + decides whether an *already-labelled* PR is skipped, so an + `UNKNOWN` there costs one sweep of delay rather than a wrong + label. + + Observed on a full sweep of a large ``: **11 of 39** + `mark-ready` candidates reported `UNKNOWN` at fetch time and + `dirty` at mutation time. All 11 were genuinely conflicting; the + pre-mutation guard refused every one. Without that guard they + would have entered the maintainer review queue unmergeable. - **Collaborator-authored PRs never get `draft`.** When `authors:collaborators` is active, fall back to `comment` with the same body. Row 9 / 17 / etc. emit `comment`, not `draft`, diff --git a/skills/pr-management-triage/fetch-and-batch.md b/skills/pr-management-triage/fetch-and-batch.md index 822406402..45c0a173f 100644 --- a/skills/pr-management-triage/fetch-and-batch.md +++ b/skills/pr-management-triage/fetch-and-batch.md @@ -53,7 +53,12 @@ query( committedDate statusCheckRollup { state # SUCCESS / FAILURE / PENDING / ERROR - contexts(first: 50) { + # NOTE: this page is TRUNCATED on large repos and the + # truncation is silent — see #failed-check-lists-are-truncated. + # `state` is always authoritative; the derived + # failed-check *list* is not, and must be re-derived from + # the check-runs REST API before any row that reads it. + contexts(first: 100) { nodes { __typename ... on CheckRun { name conclusion status } @@ -180,6 +185,68 @@ the rate-limit budget. The inner `first:` arguments are the dominant factor; if you need to widen them, *lower* the outer batch size first — never raise above 25 without measuring. +Note the interaction with +[Failed-check lists are truncated](#failed-check-lists-are-truncated): +trimming `contexts(first:)` to buy complexity headroom widens +that truncation window. That is an acceptable trade **only** +because the REST re-derivation is mandatory before any row reads +`failed_checks` — the rollup page is a fast path, never the +source of truth. + +### Failed-check lists are truncated + +**`statusCheckRollup.contexts` is a paginated connection, and the +page it returns is a silent prefix — not the whole set.** A +`` whose PRs run well past 100 check-runs (a large +matrix-heavy CI easily does) overflows any single page. Nothing in +the response signals the truncation: you get a well-formed list +that happens to be missing entries. + +`statusCheckRollup.state` is unaffected — it is computed +server-side over *all* contexts, so `SUCCESS` / `FAILURE` stays +authoritative. What is **not** authoritative is the derived +`failed_checks` list, and that list is what the decision table +reads for every CI-shaped row. + +Observed on a full 333-PR sweep of a large `` at +`first: 50`, three PRs were misrouted by the truncated list: + +| Failures per rollup page | Actual failures | Effect | +|---|---|---| +| 1 (a static check) | 16, including a whole provider-test sweep | routed to `comment` instead of `draft` | +| 2 | 4 — the same suite failing on **all four** DB backends | routed to `rerun`; a consistent cross-backend failure treated as a flake | +| 1 | 3 | routed to `comment` instead of `draft` | + +Raising the page size shrinks the window but does not close it — +a repo can always exceed it. **Before evaluating any row that +reads `failed_checks`** (rows 10, 11, 12, 12b, 13, and the +[Real-CI guard](classify-and-act.md#real-ci-guard)), re-derive the +list from the paginated check-runs REST endpoint: + +```bash +# Walk every page — stop when a page returns < 100 entries. +page=1 +while :; do + batch=$(gh api "repos///commits/${head_sha}/check-runs?per_page=100&page=${page}" \ + --jq '[.check_runs[] | select(.conclusion == "failure" or .conclusion == "timed_out") | .name]') + echo "$batch" + [ "$(gh api "repos///commits/${head_sha}/check-runs?per_page=100&page=${page}" \ + --jq '.check_runs | length')" -lt 100 ] && break + page=$((page + 1)) +done +``` + +Cost is one REST call per ~100 check-runs per PR, and only for +PRs whose `rollup.state` is `FAILURE` — green PRs skip it +entirely. On the 333-PR sweep above that was ~50 extra calls, +negligible against the 5000/h budget and far cheaper than posting +a wrong violations list to a contributor. + +**Do not** report a violations list built from the rollup page +alone. A contributor told they have "one lint failure" when they +have sixteen will fix the lint, push, and land back in triage — +having been actively misled by us. + ### `gh` invocation ```bash @@ -519,7 +586,11 @@ query($owner: String!, $repo: String!) { commit { oid statusCheckRollup { - contexts(first: 50) { + # Same truncation caveat as the main query. Here it only + # under-populates `recent_main_failures`, which makes rows + # 10/11 fire less often — a PR gets `draft`/`comment` + # instead of `rerun`. That is the safe direction to fail. + contexts(first: 100) { nodes { __typename ... on CheckRun { name conclusion } diff --git a/skills/pr-management-triage/stale-sweeps.md b/skills/pr-management-triage/stale-sweeps.md index 0e95e1ca8..327dc2644 100644 --- a/skills/pr-management-triage/stale-sweeps.md +++ b/skills/pr-management-triage/stale-sweeps.md @@ -154,7 +154,7 @@ above. Two sub-cases, both resulting in `close`: -### 1a. Triaged draft with no author reply ≥ 7 days +### 1a. Triaged draft with no author response ≥ 7 days **Trigger.** @@ -163,13 +163,34 @@ Two sub-cases, both resulting in `close`: [Ready-label exclusion](#ready-label-exclusion-applies-to-sweeps-13) — Sweep 4's domain) - `last_triage_comment_at` is not null - ` - last_triage_comment_at >= 7 days` -- No comment by the author after `last_triage_comment_at` +- `last_author_activity_at <= last_triage_comment_at` — i.e. **no + author response of any kind** since we asked. Use the + [`last_author_activity_at`](#inputs) input defined above, which + already folds in pushes and review-thread replies alongside + issue comments. + +**A push is a response.** Many contributors answer review feedback +with code and never write a comment. Testing this trigger against +*comments only* marks those authors silent while they are actively +working — and this sweep's action is `close`, the least reversible +thing the skill does. + +Observed on a full sweep of a large ``: of 3 PRs +matching a comments-only reading of this trigger, **one had author +pushes 5, 12, and 25 days after the triage comment** — actively +worked, zero comments. It would have been closed. A second had +last pushed 32 days earlier; only the third was genuinely +silent. + +The `last_author_activity_at` input exists precisely for this and +costs no extra fetch — the trigger must not fall back to a +bare comment scan. **Action.** `close` — post the [stale-draft-close](comment-templates.md#stale-draft-close) comment, then close. No label (these are not quality-violation closes). -**Reason string.** *"Draft triaged N days ago, no author reply — close with stale-draft notice"*. +**Reason string.** *"Draft triaged N days ago, no author reply or push — close with stale-draft notice"*. ### 1b. Untriaged draft with no activity ≥ 2 weeks From cac2842ff10a748e962cd77cf840b6a8650d4b50 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Sat, 8 Aug 2026 00:58:52 +0800 Subject: [PATCH 2/2] fixup: handle UNKNOWN in the rebase pre-flight guard too The mark-ready guard this PR adds refuses on both dirty and unknown. The rebase guard, which row 16 depends on, refused only on CONFLICTING and proceeded on UNKNOWN. That is the same gap one level down: this PR's own premise is that the mergeability read can return UNKNOWN, and the rebase guard's live re-query is subject to exactly the same lazy computation as the batch fetch. Proceeding spends a 'gh pr update-branch' round-trip that 422s on precisely the PRs the guard exists to catch -- self-limiting rather than contributor-facing, but the fix is two lines and makes the pattern uniform across both mutation guards. Also records why the two remaining '!= CONFLICTING' readers keep the looser form, so the next reader does not have to re-derive it: - Row 16 stays loose because its mutation is guarded (now properly). - unresolved_threads_only is diagnostic -- it selects which reason string is reported, not which action fires, so an UNKNOWN mislabels a reason rather than producing a wrong outcome. F4 was already justified in the PR. Generated-by: Claude Code (Opus 5) --- skills/pr-management-triage/actions.md | 8 ++++++++ skills/pr-management-triage/classify-and-act.md | 14 ++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/skills/pr-management-triage/actions.md b/skills/pr-management-triage/actions.md index 11d33f7e6..8e1d2daca 100644 --- a/skills/pr-management-triage/actions.md +++ b/skills/pr-management-triage/actions.md @@ -614,6 +614,14 @@ if [ "$merg" = "CONFLICTING" ]; then echo "refuse: CONFLICTING — route to draft instead" >&2 exit 2 fi +# Same lazy-computation caveat as the mark-ready guard: this live +# re-query can itself return UNKNOWN, and UNKNOWN is not "no +# conflict". Proceeding spends a round-trip that 422s on exactly the +# PRs this guard exists to catch. +if [ "$merg" = "UNKNOWN" ]; then + echo "refuse: mergeability not yet computed — retry next sweep" >&2 + exit 2 +fi ``` When the guard passes, single mutation via `gh`: diff --git a/skills/pr-management-triage/classify-and-act.md b/skills/pr-management-triage/classify-and-act.md index 22957659b..f6ee8ca25 100644 --- a/skills/pr-management-triage/classify-and-act.md +++ b/skills/pr-management-triage/classify-and-act.md @@ -138,6 +138,20 @@ Action verbs are defined in [`actions.md`](actions.md). `UNKNOWN` there costs one sweep of delay rather than a wrong label. + Row 16 also keeps `!= CONFLICTING`, but for a different reason: + it routes to `rebase`, whose own + [pre-flight guard](actions.md#rebase--update-the-pr-branch-with-base) + re-queries `mergeable` live and refuses on both `CONFLICTING` + and `UNKNOWN`. The classification stays loose because the + mutation is guarded; the guard has to handle `UNKNOWN` for that + to hold, since the live re-query is subject to the same lazy + computation as the batch fetch. + + `unresolved_threads_only` also reads `!= CONFLICTING`. That one + is diagnostic — it decides which *reason* is reported, not which + action fires — so an `UNKNOWN` mislabels a reason string rather + than producing a wrong outcome. + Observed on a full sweep of a large ``: **11 of 39** `mark-ready` candidates reported `UNKNOWN` at fetch time and `dirty` at mutation time. All 11 were genuinely conflicting; the