Merge 738f984d33 into 27b4e2d147

.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,99 @@ 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 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] | None,
+) -> 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 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"
+    if behind_by is None:
+        return "unknown"
+    if behind_by <= 0:
+        return "current"
+    if base_files is None:
+        return "unknown"
+
+    moved = {str(path) for path in base_files}
+    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, "
+            "empty when the comparison was unavailable"
+        ),
+    )
     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":
+        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
 
 

.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,47 @@ 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.
+            # 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 "$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 \
+              --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 +253,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

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,138 @@ 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
+    )
+
+
+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"

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:
@@ -30,3 +37,85 @@ 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
+
+
+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