From d229eec1ce5aab3f0325d189dfdb68c1b15b6154 Mon Sep 17 00:00:00 2001 From: Parideboy Date: Mon, 24 Aug 2026 00:25:16 +0200 Subject: [PATCH 1/2] ci(pr-health): flag stale PR branches by ref comparison The `status: needs rebase` label was driven by `mergeStateStatus == BEHIND`, which GitHub only reports when the base branch requires branches to be up to date. `main` does not, so the label has never been applied and stale-base risk stays invisible on the PR (see #3066). Compare the base branch against the PR head directly instead, and label a PR only when it is behind *and* the base moved on files the PR also changes. That is the case that hides a semantic merge conflict from per-PR checks; being one commit behind a busy base branch is not, so plain "behind" would label almost every open PR. Co-authored-by: Claude Opus 5 --- .github/scripts/pr-health-labels.py | 65 ++++++++++++++++++- .github/workflows/pr-health.yml | 36 +++++++++-- scripts/tests/test_pr_health_labels.py | 81 ++++++++++++++++++++++++ scripts/tests/test_pr_health_workflow.py | 15 +++++ 4 files changed, 191 insertions(+), 6 deletions(-) diff --git a/.github/scripts/pr-health-labels.py b/.github/scripts/pr-health-labels.py index dfa9eaacc..c9dbafe8b 100644 --- a/.github/scripts/pr-health-labels.py +++ b/.github/scripts/pr-health-labels.py @@ -6,6 +6,7 @@ from __future__ import annotations import argparse import json import sys +from collections.abc import Iterable from datetime import datetime, timezone from typing import Any @@ -63,15 +64,77 @@ def check_state(payload: dict[str, Any]) -> str: return "passing" +def parse_behind_by(value: str) -> int | None: + """Read the commit count from a compare call that may have failed.""" + try: + return int(value) + except (TypeError, ValueError): + return None + + +def drift_state( + payload: dict[str, Any], + behind_by: int | None, + base_files: Iterable[Any], +) -> str: + """Classify a pull request branch against the current tip of its base branch. + + GitHub only reports `mergeStateStatus == BEHIND` when the base branch requires + branches to be up to date, so drift is measured from an explicit ref comparison + instead. Being behind alone is normal on a busy base branch; the risky case is + being behind on files the pull request also modifies, because that is where a + semantic merge conflict hides from per-pull-request checks. + + Returns "stale", "current", or "unknown" when the comparison is unavailable. + """ + if _merge_state(payload) == "BEHIND": + return "stale" + if behind_by is None: + return "unknown" + if behind_by <= 0: + return "current" + + moved = {str(path) for path in base_files or []} + touched = { + str(entry.get("path")) for entry in payload.get("files") or [] if isinstance(entry, dict) + } + return "stale" if moved & touched else "current" + + +def _merge_state(payload: dict[str, Any]) -> str: + return str(payload.get("mergeStateStatus") or "").upper() + + def parse_args(argv: list[str]) -> argparse.Namespace: parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("--state-json", required=True, help="JSON from gh pr view") + parser.add_argument( + "--field", + choices=("checks", "drift"), + default="checks", + help="Which label signal to print", + ) + parser.add_argument( + "--behind-by", + default="", + help="Commits the base branch is ahead of the pull request head, empty when unknown", + ) + parser.add_argument( + "--base-files", + default="[]", + help="JSON array of files the base branch changed since the merge base", + ) return parser.parse_args(argv) def main(argv: list[str] | None = None) -> int: args = parse_args(argv or sys.argv[1:]) - print(check_state(json.loads(args.state_json))) + payload = json.loads(args.state_json) + if args.field == "drift": + base_files = json.loads(args.base_files or "[]") + print(drift_state(payload, parse_behind_by(args.behind_by), base_files)) + else: + print(check_state(payload)) return 0 diff --git a/.github/workflows/pr-health.yml b/.github/workflows/pr-health.yml index 4cc6ba24b..27b8a60dc 100644 --- a/.github/workflows/pr-health.yml +++ b/.github/workflows/pr-health.yml @@ -162,7 +162,7 @@ jobs: gh label create "status: needs rebase" \ --repo "$REPO" \ --color "fbca04" \ - --description "Pull request branch is behind the base branch" \ + --description "Pull request branch is behind the base branch on files it also changes" \ --force gh label create "status: has conflicts" \ --repo "$REPO" \ @@ -197,16 +197,42 @@ jobs: for pr in $pr_numbers; do data="$(gh pr view "$pr" --repo "$REPO" \ - --json isDraft,labels,mergeStateStatus,reviewDecision,statusCheckRollup)" + --json baseRefName,files,headRefOid,isDraft,labels,mergeStateStatus,reviewDecision,statusCheckRollup)" merge_state="$(jq -r '.mergeStateStatus // "UNKNOWN"' <<<"$data")" check_state="$(python3 .github/scripts/pr-health-labels.py --state-json "$data")" is_draft="$(jq -r '.isDraft' <<<"$data")" review_decision="$(jq -r '.reviewDecision // ""' <<<"$data")" + base_ref="$(jq -r '.baseRefName // ""' <<<"$data")" + head_oid="$(jq -r '.headRefOid // ""' <<<"$data")" - if [[ "$merge_state" == "BEHIND" ]]; then + # `mergeStateStatus` only reports BEHIND when the base branch requires + # branches to be up to date, so compare the refs directly instead. + head_compare='{}' + if [[ -n "$base_ref" && -n "$head_oid" ]]; then + head_compare="$(gh api "repos/$REPO/compare/$base_ref...$head_oid" \ + --jq '{behind_by: .behind_by, merge_base: .merge_base_commit.sha}' \ + 2>/dev/null || echo '{}')" + fi + behind_by="$(jq -r '.behind_by // ""' <<<"$head_compare")" + merge_base="$(jq -r '.merge_base // ""' <<<"$head_compare")" + + # Files the base branch changed since this pull request forked off it. + base_files='[]' + if [[ -n "$merge_base" && -n "$behind_by" && "$behind_by" != "0" ]]; then + base_files="$(gh api "repos/$REPO/compare/$merge_base...$base_ref" \ + --jq '[.files[]? | .filename]' 2>/dev/null || echo '[]')" + fi + + drift="$(python3 .github/scripts/pr-health-labels.py \ + --state-json "$data" \ + --field drift \ + --behind-by "$behind_by" \ + --base-files "$base_files")" + + if [[ "$drift" == "stale" ]]; then gh pr edit "$pr" --repo "$REPO" --add-label "status: needs rebase" - elif [[ "$merge_state" != "UNKNOWN" ]]; then + elif [[ "$drift" == "current" ]]; then gh pr edit "$pr" --repo "$REPO" --remove-label "status: needs rebase" || true fi @@ -222,7 +248,7 @@ jobs: gh pr edit "$pr" --repo "$REPO" --remove-label "status: ci failing" || true fi - if [[ "$merge_state" == "BEHIND" || "$merge_state" == "DIRTY" || "$check_state" == "failing" || "$is_draft" == "true" || "$review_decision" == "CHANGES_REQUESTED" ]]; then + if [[ "$drift" == "stale" || "$merge_state" == "DIRTY" || "$check_state" == "failing" || "$is_draft" == "true" || "$review_decision" == "CHANGES_REQUESTED" ]]; then gh pr edit "$pr" --repo "$REPO" --remove-label "status: ready for review" || true fi done diff --git a/scripts/tests/test_pr_health_labels.py b/scripts/tests/test_pr_health_labels.py index a7bb5ef18..f25e26c7f 100644 --- a/scripts/tests/test_pr_health_labels.py +++ b/scripts/tests/test_pr_health_labels.py @@ -3,6 +3,7 @@ from __future__ import annotations import importlib.util +import json import sys from pathlib import Path @@ -92,3 +93,83 @@ def test_check_state_fails_when_latest_attempt_failed() -> None: } assert module.check_state(payload) == "failing" + + +def test_drift_state_flags_branch_behind_on_files_it_also_changes() -> None: + module = _load_module() + payload = { + "mergeStateStatus": "CLEAN", + "files": [{"path": "headroom/proxy/server.py"}], + } + + base_files = ["headroom/proxy/server.py", "docs/index.md"] + + assert module.drift_state(payload, 13, base_files) == "stale" + + +def test_drift_state_ignores_base_movement_on_unrelated_files() -> None: + module = _load_module() + payload = { + "mergeStateStatus": "CLEAN", + "files": [{"path": "headroom/proxy/server.py"}], + } + + assert module.drift_state(payload, 13, ["docs/index.md"]) == "current" + + +def test_drift_state_is_current_when_the_branch_is_up_to_date() -> None: + module = _load_module() + payload = { + "mergeStateStatus": "CLEAN", + "files": [{"path": "headroom/proxy/server.py"}], + } + + assert module.drift_state(payload, 0, []) == "current" + + +def test_drift_state_is_unknown_when_the_comparison_failed() -> None: + module = _load_module() + payload = { + "mergeStateStatus": "UNKNOWN", + "files": [{"path": "headroom/proxy/server.py"}], + } + + assert module.drift_state(payload, None, []) == "unknown" + + +def test_drift_state_still_trusts_a_behind_merge_state() -> None: + module = _load_module() + payload = {"mergeStateStatus": "BEHIND", "files": []} + + assert module.drift_state(payload, None, []) == "stale" + + +def test_parse_behind_by_reads_an_empty_comparison_as_unknown() -> None: + module = _load_module() + + assert module.parse_behind_by("13") == 13 + assert module.parse_behind_by("") is None + assert module.parse_behind_by("null") is None + + +def test_main_prints_the_drift_state() -> None: + module = _load_module() + state_json = json.dumps( + {"mergeStateStatus": "CLEAN", "files": [{"path": "headroom/proxy/server.py"}]} + ) + + assert ( + module.main( + [ + "--state-json", + state_json, + "--field", + "drift", + "--behind-by", + "13", + "--base-files", + json.dumps(["headroom/proxy/server.py"]), + ] + ) + == 0 + ) diff --git a/scripts/tests/test_pr_health_workflow.py b/scripts/tests/test_pr_health_workflow.py index c317cae04..5217cf60d 100644 --- a/scripts/tests/test_pr_health_workflow.py +++ b/scripts/tests/test_pr_health_workflow.py @@ -30,3 +30,18 @@ def test_merge_state_unknown_does_not_clear_conflict_or_rebase_labels() -> None: assert 'elif [[ "$merge_state" != "UNKNOWN" ]]; then' in workflow assert 'gh pr edit "$pr" --repo "$REPO" --remove-label "status: needs rebase"' in workflow assert 'gh pr edit "$pr" --repo "$REPO" --remove-label "status: has conflicts"' in workflow + + +def test_rebase_label_uses_ref_comparison_instead_of_merge_state() -> None: + workflow = Path(".github/workflows/pr-health.yml").read_text(encoding="utf-8") + + assert 'if [[ "$merge_state" == "BEHIND" ]]; then' not in workflow + assert 'gh api "repos/$REPO/compare/$base_ref...$head_oid"' in workflow + assert "--field drift" in workflow + assert 'if [[ "$drift" == "stale" ]]; then' in workflow + + +def test_unknown_drift_does_not_clear_the_rebase_label() -> None: + workflow = Path(".github/workflows/pr-health.yml").read_text(encoding="utf-8") + + assert 'elif [[ "$drift" == "current" ]]; then' in workflow From 738f984d33af8c24b1fdca0cdef98cab16fc9867 Mon Sep 17 00:00:00 2001 From: Parideboy Date: Thu, 27 Aug 2026 17:29:49 +0200 Subject: [PATCH 2/2] ci(pr-health): keep a failed base-file comparison from clearing the rebase label The base-file comparison fell back to `[]` when the API call failed, so `drift_state()` saw a positive `behind_by` with no moved files, found no overlap and returned `current`. The label step then removed `status: needs rebase` from a pull request that was genuinely stale, which contradicts the fail-safe behaviour the first comparison already has. The same hole existed when the first comparison answered without a merge base. The workflow now sets an empty-string sentinel before the call and resets to it when the call fails, so partial output from a call that died midway is discarded too. `parse_base_files()` maps that sentinel (and any unusable payload) to `None`, and `drift_state()` returns `unknown` when the branch is behind but the comparison never answered, which neither adds nor removes the label. A successful comparison that returns no files still classifies as `current`. Co-Authored-By: Claude Opus 5 --- .github/scripts/pr-health-labels.py | 36 +++++++++--- .github/workflows/pr-health.yml | 11 +++- scripts/tests/test_pr_health_labels.py | 55 ++++++++++++++++++ scripts/tests/test_pr_health_workflow.py | 74 ++++++++++++++++++++++++ 4 files changed, 166 insertions(+), 10 deletions(-) diff --git a/.github/scripts/pr-health-labels.py b/.github/scripts/pr-health-labels.py index c9dbafe8b..5eeffea50 100644 --- a/.github/scripts/pr-health-labels.py +++ b/.github/scripts/pr-health-labels.py @@ -72,10 +72,23 @@ def parse_behind_by(value: str) -> int | None: return None +def parse_base_files(value: str) -> list[str] | None: + """Read the compared file list from a compare call that may have failed.""" + if not value or not value.strip(): + return None + try: + parsed = json.loads(value) + except json.JSONDecodeError: + return None + if not isinstance(parsed, list): + return None + return [str(item) for item in parsed] + + def drift_state( payload: dict[str, Any], behind_by: int | None, - base_files: Iterable[Any], + base_files: Iterable[Any] | None, ) -> str: """Classify a pull request branch against the current tip of its base branch. @@ -85,7 +98,10 @@ def drift_state( being behind on files the pull request also modifies, because that is where a semantic merge conflict hides from per-pull-request checks. - Returns "stale", "current", or "unknown" when the comparison is unavailable. + Returns "stale", "current", or "unknown" when either comparison is unavailable. + A `base_files` of None means the comparison never answered, which is not the same + as a comparison that answered with no files, so the label is left alone instead of + being cleared. """ if _merge_state(payload) == "BEHIND": return "stale" @@ -93,8 +109,10 @@ def drift_state( return "unknown" if behind_by <= 0: return "current" + if base_files is None: + return "unknown" - moved = {str(path) for path in base_files or []} + moved = {str(path) for path in base_files} touched = { str(entry.get("path")) for entry in payload.get("files") or [] if isinstance(entry, dict) } @@ -121,8 +139,11 @@ def parse_args(argv: list[str]) -> argparse.Namespace: ) parser.add_argument( "--base-files", - default="[]", - help="JSON array of files the base branch changed since the merge base", + default="", + help=( + "JSON array of files the base branch changed since the merge base, " + "empty when the comparison was unavailable" + ), ) return parser.parse_args(argv) @@ -131,8 +152,9 @@ def main(argv: list[str] | None = None) -> int: args = parse_args(argv or sys.argv[1:]) payload = json.loads(args.state_json) if args.field == "drift": - base_files = json.loads(args.base_files or "[]") - print(drift_state(payload, parse_behind_by(args.behind_by), base_files)) + behind_by = parse_behind_by(args.behind_by) + base_files = parse_base_files(args.base_files) + print(drift_state(payload, behind_by, base_files)) else: print(check_state(payload)) return 0 diff --git a/.github/workflows/pr-health.yml b/.github/workflows/pr-health.yml index 27b8a60dc..1f2f88aa2 100644 --- a/.github/workflows/pr-health.yml +++ b/.github/workflows/pr-health.yml @@ -218,10 +218,15 @@ jobs: merge_base="$(jq -r '.merge_base // ""' <<<"$head_compare")" # Files the base branch changed since this pull request forked off it. + # An empty string means the comparison never produced an answer; it must + # not read as "the base moved nothing", which would clear a correct label. base_files='[]' - if [[ -n "$merge_base" && -n "$behind_by" && "$behind_by" != "0" ]]; then - base_files="$(gh api "repos/$REPO/compare/$merge_base...$base_ref" \ - --jq '[.files[]? | .filename]' 2>/dev/null || echo '[]')" + if [[ -n "$behind_by" && "$behind_by" != "0" ]]; then + base_files='' + if [[ -n "$merge_base" ]]; then + base_files="$(gh api "repos/$REPO/compare/$merge_base...$base_ref" \ + --jq '[.files[]? | .filename]' 2>/dev/null)" || base_files='' + fi fi drift="$(python3 .github/scripts/pr-health-labels.py \ diff --git a/scripts/tests/test_pr_health_labels.py b/scripts/tests/test_pr_health_labels.py index f25e26c7f..69ad5fdb7 100644 --- a/scripts/tests/test_pr_health_labels.py +++ b/scripts/tests/test_pr_health_labels.py @@ -173,3 +173,58 @@ def test_main_prints_the_drift_state() -> None: ) == 0 ) + + +def test_drift_state_is_unknown_when_the_base_file_comparison_failed() -> None: + module = _load_module() + payload = { + "mergeStateStatus": "CLEAN", + "files": [{"path": "headroom/proxy/server.py"}], + } + + assert module.drift_state(payload, 13, None) == "unknown" + + +def test_drift_state_is_current_when_the_base_moved_no_files() -> None: + module = _load_module() + payload = { + "mergeStateStatus": "CLEAN", + "files": [{"path": "headroom/proxy/server.py"}], + } + + assert module.drift_state(payload, 13, []) == "current" + + +def test_parse_base_files_reads_a_failed_comparison_as_unknown() -> None: + module = _load_module() + + assert module.parse_base_files("") is None + assert module.parse_base_files(" ") is None + assert module.parse_base_files("not json") is None + assert module.parse_base_files('{"files": []}') is None + assert module.parse_base_files("[]") == [] + assert module.parse_base_files('["headroom/proxy/server.py"]') == ["headroom/proxy/server.py"] + + +def test_main_prints_unknown_when_the_base_file_comparison_failed(capsys) -> None: + module = _load_module() + state_json = json.dumps( + {"mergeStateStatus": "CLEAN", "files": [{"path": "headroom/proxy/server.py"}]} + ) + + assert ( + module.main( + [ + "--state-json", + state_json, + "--field", + "drift", + "--behind-by", + "13", + "--base-files", + "", + ] + ) + == 0 + ) + assert capsys.readouterr().out.strip() == "unknown" diff --git a/scripts/tests/test_pr_health_workflow.py b/scripts/tests/test_pr_health_workflow.py index 5217cf60d..755825c58 100644 --- a/scripts/tests/test_pr_health_workflow.py +++ b/scripts/tests/test_pr_health_workflow.py @@ -2,7 +2,14 @@ from __future__ import annotations +import json +import shutil +import subprocess +import sys from pathlib import Path +from textwrap import dedent + +import pytest def test_incomplete_pr_template_is_reported_without_failing_job() -> None: @@ -45,3 +52,70 @@ def test_unknown_drift_does_not_clear_the_rebase_label() -> None: workflow = Path(".github/workflows/pr-health.yml").read_text(encoding="utf-8") assert 'elif [[ "$drift" == "current" ]]; then' in workflow + + +def _base_files_snippet(workflow: str) -> str: + """The shell block that asks GitHub which files the base branch moved.""" + start = workflow.index(" # Files the base branch changed") + end = workflow.index(' drift="$(python3', start) + return dedent(workflow[start:end]) + + +def test_failed_base_file_comparison_keeps_an_unknown_sentinel() -> None: + workflow = Path(".github/workflows/pr-health.yml").read_text(encoding="utf-8") + + assert "echo '[]'" not in workflow + assert "base_files=''" in workflow + assert "|| base_files=''" in workflow + + +def test_failed_base_file_comparison_does_not_clear_the_rebase_label() -> None: + bash = shutil.which("bash") + if bash is None: + pytest.skip("bash is unavailable") + + workflow = Path(".github/workflows/pr-health.yml").read_text(encoding="utf-8") + script = "\n".join( + [ + "set -euo pipefail", + # Stands in for a throttled API call that writes partial output and fails. + "gh() { printf 'partia'; return 1; }", + 'REPO="headroomlabs-ai/headroom"', + 'base_ref="main"', + 'behind_by="13"', + 'merge_base="0123456789abcdef0123456789abcdef01234567"', + _base_files_snippet(workflow), + 'printf "%s" "$base_files"', + ] + ) + result = subprocess.run([bash, "-c", script], capture_output=True, text=True) + + assert result.returncode == 0, result.stderr + assert result.stdout == "" + + drift = subprocess.run( + [ + sys.executable, + ".github/scripts/pr-health-labels.py", + "--state-json", + json.dumps( + {"mergeStateStatus": "CLEAN", "files": [{"path": "headroom/proxy/server.py"}]} + ), + "--field", + "drift", + "--behind-by", + "13", + "--base-files", + result.stdout, + ], + capture_output=True, + text=True, + ) + + assert drift.returncode == 0, drift.stderr + assert drift.stdout.strip() == "unknown" + # An unknown verdict matches neither branch, so no gh pr edit runs for the label. + start = workflow.index(' if [[ "$drift" == "stale" ]]; then') + dispatch = workflow[start : workflow.index("\n fi\n", start)] + assert 'elif [[ "$drift" == "current" ]]; then' in dispatch + assert "else" not in dispatch