From e1a9f323636195517417183fb486984e5d7abc35 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Mon, 20 Jul 2026 16:42:13 +0200 Subject: [PATCH 1/3] 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/3] 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 From 605b189fd6342ea2abf6e2c4818d17a41e484bb1 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Wed, 12 Aug 2026 16:18:58 +0200 Subject: [PATCH 3/3] fixup: keep contexts at 50, single-fetch pagination, cover UNKNOWN in the table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on this branch. - Revert `contexts(first:)` to 50 in both queries. Raising it to 100 cut against the Batch-size section directly above, which is unchanged and says the inner `first:` args are the dominant complexity factor and that widening them means lowering `$batchSize` first. It also bought nothing: this same branch makes the REST re-derivation mandatory, so the rollup page is explicitly no longer a source of truth, and a larger page cannot close the truncation window either way. Documented the reasoning at both call sites and in the Batch-size section. - Fetch each check-runs page once. The pagination recipe issued two identical `gh api` calls per page — the second only to read `.check_runs | length` — which doubles the call budget the same section advertises as "one REST call per ~100 check-runs". - Give `mergeable == UNKNOWN` a row. Tightening rows 19/20 to `== MERGEABLE` left UNKNOWN matching *no* row at all: it also fails `has_deterministic_signal`, so it never reaches the row 17 fallback, and the prose claim that it "falls through to skip" had nothing behind it. Row 22 already means "server-side state not yet settled, retry" and is already evaluated before rows 17 and 19-20, so it is the right home. - Sync the eval suite. `decision-table/fixtures/system-prompt.md` carries its own copy of the table and still said `mergeable != CONFLICTING`; rows 19/20/22 updated to match. Added `case-19-unknown-mergeable` with a `has_reason_mergeability_unsettled` structural assertion, so landing on `skip` for the wrong reason fails the case. Generated-by: Claude Code (Opus 5) --- .../pr-management-triage/classify-and-act.md | 17 ++++--- .../pr-management-triage/fetch-and-batch.md | 45 ++++++++++++------- .../decision-table/fixtures/assertions.json | 3 +- .../case-19-unknown-mergeable/expected.json | 5 +++ .../case-19-unknown-mergeable/report.md | 15 +++++++ .../decision-table/fixtures/system-prompt.md | 10 +++-- 6 files changed, 70 insertions(+), 25 deletions(-) create mode 100644 tools/skill-evals/evals/pr-management-triage/decision-table/fixtures/case-19-unknown-mergeable/expected.json create mode 100644 tools/skill-evals/evals/pr-management-triage/decision-table/fixtures/case-19-unknown-mergeable/report.md diff --git a/skills/pr-management-triage/classify-and-act.md b/skills/pr-management-triage/classify-and-act.md index f6ee8ca25..3d86936ba 100644 --- a/skills/pr-management-triage/classify-and-act.md +++ b/skills/pr-management-triage/classify-and-act.md @@ -103,7 +103,7 @@ Action verbs are defined in [`actions.md`](actions.md). | 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 @@ -123,11 +123,16 @@ Action verbs are defined in [`actions.md`](actions.md). `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. + - 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 diff --git a/skills/pr-management-triage/fetch-and-batch.md b/skills/pr-management-triage/fetch-and-batch.md index 45c0a173f..ae41f4ad8 100644 --- a/skills/pr-management-triage/fetch-and-batch.md +++ b/skills/pr-management-triage/fetch-and-batch.md @@ -58,7 +58,11 @@ query( # `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) { + # 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 ... on CheckRun { name conclusion status } @@ -193,14 +197,22 @@ 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 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. +`` 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 @@ -225,13 +237,15 @@ 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 - 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_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 ``` @@ -586,11 +600,12 @@ query($owner: String!, $repo: String!) { commit { oid statusCheckRollup { - # 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) { + # 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 ... on CheckRun { name conclusion } 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 f8ae8c84d..4a8e04832 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 000000000..8c99c361f --- /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 000000000..6fbb63540 --- /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 a87d8a5cd..a26b7965a 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