diff --git a/skills/pr-management-triage/actions.md b/skills/pr-management-triage/actions.md index 531aa0b4..8e1d2dac 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 @@ -592,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 6a611c7c..3d86936b 100644 --- a/skills/pr-management-triage/classify-and-act.md +++ b/skills/pr-management-triage/classify-and-act.md @@ -100,10 +100,10 @@ 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 | +| 22 | Unsettled server-side state, either: (a) 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); or (b) `mergeable == UNKNOWN` — GitHub has not finished computing mergeability. Evaluated **before** rows 17, 19-20 — see [hard rules](#hard-rules-cross-cutting-the-table) | n/a | `skip` | State not yet settled, retry next sweep | ### Hard rules cross-cutting the table @@ -116,6 +116,52 @@ 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, and + **row 22 catches the `UNKNOWN` case** — without that the PR + would match no row at all, since `UNKNOWN` also fails + [`has_deterministic_signal`](#has_deterministic_signal) and so + never reaches the row 17 fallback. Row 22 is the right home: + it already means *"server-side state not yet settled, retry"* + and is already evaluated before rows 17 and 19-20. The PR + skips 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. + + 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 + 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 82240640..ae41f4ad 100644 --- a/skills/pr-management-triage/fetch-and-batch.md +++ b/skills/pr-management-triage/fetch-and-batch.md @@ -53,6 +53,15 @@ query( committedDate statusCheckRollup { state # SUCCESS / FAILURE / PENDING / ERROR + # 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. + # Kept at 50 deliberately: raising it cannot close the + # truncation window, and the REST re-derivation makes this + # page a fast path rather than a source of truth — so the + # extra complexity would buy nothing. See #batch-size. contexts(first: 50) { nodes { __typename @@ -180,6 +189,78 @@ 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. + +The same reasoning cuts the other way, and is why `contexts(first:)` +stays at 50 rather than being raised: a larger page cannot close the +truncation window either (a repo can always exceed any fixed page), +so raising it would spend complexity budget — the dominant factor +per the paragraph above — to buy a list nothing is allowed to trust. +Widen it only alongside a measured `cost=` figure and a matching +reduction in `$batchSize`. + +### 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 more check-runs than the page holds (a +large matrix-heavy CI easily does) overflows it. 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. +# Fetch each page ONCE and derive both the failure names and the page +# length from that single response; re-querying the same page to count +# it doubles the call budget this section advertises. +page=1 +while :; do + page_json=$(gh api "repos///commits/${head_sha}/check-runs?per_page=100&page=${page}") + jq -r '.check_runs[] | select(.conclusion == "failure" or .conclusion == "timed_out") | .name' \ + <<<"$page_json" + [ "$(jq '.check_runs | length' <<<"$page_json")" -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,6 +600,11 @@ query($owner: String!, $repo: String!) { commit { oid statusCheckRollup { + # Same truncation caveat as the main query, and kept at 50 + # for the same reason. 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: 50) { nodes { __typename diff --git a/skills/pr-management-triage/stale-sweeps.md b/skills/pr-management-triage/stale-sweeps.md index 0e95e1ca..327dc264 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 diff --git a/tools/skill-evals/evals/pr-management-triage/decision-table/fixtures/assertions.json b/tools/skill-evals/evals/pr-management-triage/decision-table/fixtures/assertions.json index f8ae8c84..4a8e0483 100644 --- a/tools/skill-evals/evals/pr-management-triage/decision-table/fixtures/assertions.json +++ b/tools/skill-evals/evals/pr-management-triage/decision-table/fixtures/assertions.json @@ -1,3 +1,4 @@ { - "has_reason_no_real_ci": {"type": "regex", "field": "reason", "pattern": "(no|not|without).{0,30}(real )?ci|ci.{0,20}(not|never).{0,20}(trigger|ran|run)|re-?trigger", "flags": "i"} + "has_reason_no_real_ci": {"type": "regex", "field": "reason", "pattern": "(no|not|without).{0,30}(real )?ci|ci.{0,20}(not|never).{0,20}(trigger|ran|run)|re-?trigger", "flags": "i"}, + "has_reason_mergeability_unsettled": {"type": "regex", "field": "reason", "pattern": "unknown|not (yet )?(been )?(computed|determined|settled)|undetermined|unsettled|still computing|retry next sweep", "flags": "i"} } diff --git a/tools/skill-evals/evals/pr-management-triage/decision-table/fixtures/case-19-unknown-mergeable/expected.json b/tools/skill-evals/evals/pr-management-triage/decision-table/fixtures/case-19-unknown-mergeable/expected.json new file mode 100644 index 00000000..8c99c361 --- /dev/null +++ b/tools/skill-evals/evals/pr-management-triage/decision-table/fixtures/case-19-unknown-mergeable/expected.json @@ -0,0 +1,5 @@ +{ + "classification": null, + "action": "skip", + "has_reason_mergeability_unsettled": true +} diff --git a/tools/skill-evals/evals/pr-management-triage/decision-table/fixtures/case-19-unknown-mergeable/report.md b/tools/skill-evals/evals/pr-management-triage/decision-table/fixtures/case-19-unknown-mergeable/report.md new file mode 100644 index 00000000..6fbb6354 --- /dev/null +++ b/tools/skill-evals/evals/pr-management-triage/decision-table/fixtures/case-19-unknown-mergeable/report.md @@ -0,0 +1,15 @@ + + +PR #14931 +Author: rowan-contributor +AuthorAssociation: CONTRIBUTOR +StatusCheckRollup: SUCCESS +FailedChecks: [] +RecentMainFailures: [] +Mergeable: UNKNOWN +UnresolvedThreads: 0 +IsDraft: false +CommitsBehind: 3 +RealCIRan: true +Labels: ["area:core"] diff --git a/tools/skill-evals/evals/pr-management-triage/decision-table/fixtures/system-prompt.md b/tools/skill-evals/evals/pr-management-triage/decision-table/fixtures/system-prompt.md index a87d8a5c..a26b7965 100644 --- a/tools/skill-evals/evals/pr-management-triage/decision-table/fixtures/system-prompt.md +++ b/tools/skill-evals/evals/pr-management-triage/decision-table/fixtures/system-prompt.md @@ -72,10 +72,14 @@ and reason. | 16 | No real CI checks triggered AND `mergeable != CONFLICTING` AND author is NOT first-time (`authorAssociation` NOT IN {`FIRST_TIME_CONTRIBUTOR`, `FIRST_TIMER`}) | `deterministic_flag` | `rebase` | | 17 | `has_deterministic_signal` (fallback) | `deterministic_flag` | `draft` | | 18 | `latestReviews` has CHANGES_REQUESTED AND author pushed commits after that review AND NOT `follow_up_ping` | `stale_review` | `ping` | -| 19 | `statusCheckRollup == SUCCESS` AND `mergeable != CONFLICTING` AND `unresolved_threads == 0` AND real CI ran AND labels contain `ready for maintainer review` | `passing` | `skip` | -| 20 | `statusCheckRollup == SUCCESS` AND `mergeable != CONFLICTING` AND `unresolved_threads == 0` AND real CI ran | `passing` | `mark-ready` | +| 19 | `statusCheckRollup == SUCCESS` AND `mergeable == MERGEABLE` AND `unresolved_threads == 0` AND real CI ran AND labels contain `ready for maintainer review` | `passing` | `skip` | +| 20 | `statusCheckRollup == SUCCESS` AND `mergeable == MERGEABLE` AND `unresolved_threads == 0` AND real CI ran | `passing` | `mark-ready` | | 21 | Stale sweep candidate — no row 1–20 matched AND PR meets stale criteria: `isDraft == true` AND triage marker exists AND `(now - triage_comment_at) >= 7 days` AND `(now - last_author_activity) >= 14 days` | `stale_draft` | `close` | -| 22 | Data anomaly — `statusCheckRollup == SUCCESS` but `failed_checks` is non-empty, OR `statusCheckRollup == FAILURE` but `failed_checks` is empty. Evaluated before rows 17, 19, 20. | n/a | `skip` | +| 22 | Unsettled server-side state — `statusCheckRollup == SUCCESS` but `failed_checks` is non-empty, OR `statusCheckRollup == FAILURE` but `failed_checks` is empty, OR `mergeable == UNKNOWN`. Evaluated before rows 17, 19, 20. | n/a | `skip` | + +Note on rows 19/20: `mergeable == UNKNOWN` means GitHub has not +finished computing mergeability. It is **not** the same as "no +conflict" — treat it as undetermined and let row 22 handle it. ## Output