From 44d5a56289228668e67910ea33fd73bde0362701 Mon Sep 17 00:00:00 2001 From: Andrew Halberstadt Date: Tue, 10 Nov 2026 15:23:29 -0500 Subject: [PATCH] feat(run-task): derive base_rev from the GitHub compare API --- .taskcluster.yml | 2 +- docs/reference/parameters.rst | 3 + docs/reference/run-task.rst | 18 ++ src/taskgraph/run-task/run-task | 123 ++++++++++++++ test/test_scripts_run_task.py | 284 ++++++++++++++++++++++++++++++++ 5 files changed, 429 insertions(+), 1 deletion(-) diff --git a/.taskcluster.yml b/.taskcluster.yml index f7380fd31..357ca4162 100644 --- a/.taskcluster.yml +++ b/.taskcluster.yml @@ -287,7 +287,7 @@ tasks: --tasks-for='${tasks_for}' --base-repository='${baseRepoUrl}' --base-ref='${base_ref}' - --base-rev='${base_sha}' + --base-rev="$TASKGRAPH_BASE_REV" --head-repository='${repoUrl}' --head-ref='${head_ref}' --head-rev='${head_sha}' diff --git a/docs/reference/parameters.rst b/docs/reference/parameters.rst index ebb32eb3a..29a04e433 100644 --- a/docs/reference/parameters.rst +++ b/docs/reference/parameters.rst @@ -28,6 +28,9 @@ base_rev ~~~~~~~~ The previous revision before ``head_rev`` got merged into. This can be a short revision string. + For ``github-push`` and ``github-pull-request`` tasks_for, ``run-task`` can replace the + reported base with the merge base. See :ref:`run-task-base-revisions`. + head_rev ~~~~~~~~ The revision to check out, this can be a short revision string. diff --git a/docs/reference/run-task.rst b/docs/reference/run-task.rst index 5a24d49c0..de9a102ce 100644 --- a/docs/reference/run-task.rst +++ b/docs/reference/run-task.rst @@ -48,3 +48,21 @@ token. The same happens when the auth service rejects the request, such as for an unknown ``app`` or invalid ``permissions``. Invalid JSON, an unknown ``type`` or key, or a ``repo`` that isn't on GitHub fails the task. Installation tokens expire after an hour. + +.. _run-task-base-revisions: + +GitHub base revisions +--------------------- + +GitHub often reports the wrong base revision: the null revision for a new +branch, the orphaned old tip for a force push, and the target's current tip for +a pull request. For a GitHub git checkout that sets ``_HEAD_REV`` and +either ``_BASE_REV`` or ``_BASE_REF``, ``run-task`` uses GitHub's +compare API to find the right base. It then fetches that commit and exports it +as ``_BASE_REV``. The requests are authenticated with a +``github-repo-token`` for the repository from :ref:`run-task-secrets` if there +is one, or else with ``GITHUB_TOKEN`` if it is set. If the lookup fails, the +reported base is kept. + +Every task that sets ``_BASE_REV`` or ``_BASE_REF`` on a GitHub +repository makes these compare requests implicitly. diff --git a/src/taskgraph/run-task/run-task b/src/taskgraph/run-task/run-task index 54b1ddfa0..e81d3ba5a 100755 --- a/src/taskgraph/run-task/run-task +++ b/src/taskgraph/run-task/run-task @@ -33,6 +33,7 @@ import threading import time import traceback import urllib.error +import urllib.parse import urllib.request from pathlib import Path from typing import Dict, List, Literal, NamedTuple, Optional @@ -1240,6 +1241,96 @@ def fetch_secrets(): ) +def github_compare(owner, repo, base, head): + """Return GitHub's comparison status and merge base, or None on failure.""" + base = urllib.parse.quote(base, safe="") + head = urllib.parse.quote(head, safe="") + request = urllib.request.Request( + f"https://api.github.com/repos/{owner}/{repo}/compare/{base}...{head}?per_page=1" + ) + # Prefer a token minted for this repository from RUN_TASK_SECRETS, wherever + # it was exported to, then one the task provides. + token = GITHUB_TOKENS.get((owner.lower(), repo.lower())) or os.environ.get( + "GITHUB_TOKEN" + ) + if token: + request.add_header("Authorization", f"Bearer {token}") + try: + with urllib.request.urlopen(request, timeout=10) as response: + comparison = json.load(response) + status = comparison["status"] + merge_base = comparison["merge_base_commit"]["sha"] + if status not in ( + "ahead", + "behind", + "diverged", + "identical", + ) or not re.fullmatch(r"[a-f0-9]{40}", merge_base): + raise ValueError("Invalid GitHub comparison") + return status, merge_base + except (urllib.error.URLError, OSError, ValueError, KeyError, TypeError): + print_line( + b"vcs", + b"warning: GitHub comparison failed; keeping reported base revision\n", + ) + return None + + +def _default_branch(repo_url): + """Resolve the remote's default branch without a GitHub API request.""" + try: + output = subprocess.check_output( + ["git", "ls-remote", "--symref", repo_url, "HEAD"], + universal_newlines=True, + timeout=60, + ) + match = re.search(r"ref:\s+refs/heads/(?P\S+)\s+HEAD", output) + if match: + return match.group("branch_name") + except (OSError, subprocess.SubprocessError): + pass + print_line( + b"vcs", + b"warning: unable to resolve default branch; keeping reported base revision\n", + ) + return None + + +def derive_base_rev( + github_repo, base_repo, head_repo, base_rev, base_ref, head_ref, head_rev +): + """Find the base of the pushed or proposed changes using GitHub comparisons. + + ``github_repo`` is the ``(owner, name)`` of the repository to compare in. + """ + if head_ref and head_ref.startswith("refs/tags/"): + return None + + comparison = None + if base_rev and base_rev != NULL_REVISION: + comparison = github_compare(*github_repo, base_rev, head_rev) + if comparison is None or comparison[0] in ("ahead", "identical"): + return None + + same_repo = base_repo == head_repo + target = base_ref + if target and not (same_repo and shortref(target) == shortref(head_ref or "")): + # The reported base is the tip of base_ref, as for a pull request, so + # the comparison above already found the merge base with the target. + if comparison: + return comparison[1] + else: + target = _default_branch(base_repo or head_repo) + if not target: + return None + + if same_repo and shortref(target) == shortref(head_ref or ""): + return comparison[1] if comparison else None + + comparison = github_compare(*github_repo, target, head_rev) + return comparison[1] if comparison else None + + def fetch_ssh_secret(secret_name): """Retrieves the private ssh key, and returns it as a StringIO object""" secret_url = SECRET_BASEURL_TPL.format(secret_name) @@ -1397,6 +1488,9 @@ def collect_vcs_options(args, project, name): repo_type = os.environ.get(f"{env_prefix}_REPOSITORY_TYPE") base_repo = os.environ.get(f"{env_prefix}_BASE_REPOSITORY") base_rev = os.environ.get(f"{env_prefix}_BASE_REV") + # Only used to pick the branch to compare against when deriving base_rev. + # base_ref is deliberately never fetched or checked out. + base_ref = os.environ.get(f"{env_prefix}_BASE_REF") head_repo = os.environ.get(f"{env_prefix}_HEAD_REPOSITORY") head_ref = os.environ.get(f"{env_prefix}_HEAD_REF") head_rev = os.environ.get(f"{env_prefix}_HEAD_REV") @@ -1428,6 +1522,7 @@ def collect_vcs_options(args, project, name): "checkout": checkout, "base-repo": base_repo, "base-rev": base_rev, + "base-ref": base_ref, "head-repo": head_repo, "head-ref": head_ref, "head-rev": head_rev, @@ -1451,6 +1546,32 @@ def vcs_checkout_from_args(options): ) return + github_repo = _github_owner_repo(options["base-repo"]) or _github_owner_repo( + options["head-repo"] + ) + derive_base = ( + options["repo-type"] == "git" + and github_repo + and options["head-rev"] + and (options["base-rev"] or options.get("base-ref")) + ) + if derive_base: + base_rev = derive_base_rev( + github_repo, + options["base-repo"], + options["head-repo"], + options["base-rev"], + options.get("base-ref"), + options["head-ref"], + options["head-rev"], + ) + if base_rev: + print_line( + b"vcs", + f"derived base revision: {options['base-rev']} -> {base_rev}\n".encode(), + ) + options["base-rev"] = base_rev + head_ref = options["head-ref"] head_rev = options["head-rev"] ssh_key_file = None @@ -1522,6 +1643,8 @@ def vcs_checkout_from_args(options): pass os.environ["{}_HEAD_REV".format(options["env-prefix"])] = revision + if derive_base: + os.environ[f"{options['env-prefix']}_BASE_REV"] = options["base-rev"] or "" def install_pip_requirements(repositories): diff --git a/test/test_scripts_run_task.py b/test/test_scripts_run_task.py index a375d700e..b0250ff53 100644 --- a/test/test_scripts_run_task.py +++ b/test/test_scripts_run_task.py @@ -177,6 +177,7 @@ def test_install_pip_requirements_with_uv( "REPOSITORY_TYPE": "git", "HEAD_REPOSITORY": "https://github.com/test/repo.git", "HEAD_REV": "abc123", + "BASE_REF": "refs/heads/main", }, {"shallow-clone": True}, id="git_with_shallow_clone", @@ -217,6 +218,7 @@ def test_collect_vcs_options( expected = { "base-repo": env.get("BASE_REPOSITORY"), "base-rev": env.get("BASE_REV"), + "base-ref": env.get("BASE_REF"), "checkout": os.path.join(os.getcwd(), "checkout"), "env-prefix": name.upper(), "head-repo": env.get("HEAD_REPOSITORY"), @@ -1293,3 +1295,285 @@ def test_main_exports_secrets(run_main, mocker, run_task_mod, github_tokens): assert result == 0 assert env["GITHUB_TOKEN"] == "t1" + + +@pytest.mark.parametrize( + "minted,env,expected", + [ + (None, None, None), + (None, "from-env", "from-env"), + ("minted", None, "minted"), + ("minted", "from-env", "minted"), + ], +) +def test_github_compare( + run_task_mod, monkeypatch, mocker, github_tokens, minted, env, expected +): + monkeypatch.delenv("GITHUB_TOKEN", raising=False) + if env: + monkeypatch.setenv("GITHUB_TOKEN", env) + if minted: + github_tokens[("owner", "repo")] = minted + sha = "a" * 40 + response = mocker.MagicMock() + response.__enter__.return_value.read.return_value = json.dumps( + {"status": "diverged", "merge_base_commit": {"sha": sha}} + ).encode() + urlopen = mocker.patch("urllib.request.urlopen", return_value=response) + assert run_task_mod.github_compare("owner", "repo", "refs/heads/main", "abc") == ( + "diverged", + sha, + ) + request = urlopen.call_args.args[0] + assert ( + request.full_url + == "https://api.github.com/repos/owner/repo/compare/refs%2Fheads%2Fmain...abc?per_page=1" + ) + assert request.get_method() == "GET" + assert request.get_header("Authorization") == ( + f"Bearer {expected}" if expected else None + ) + assert urlopen.call_args.kwargs == {"timeout": 10} + urlopen.side_effect = urllib.error.URLError("unavailable") + assert run_task_mod.github_compare("owner", "repo", "main", "abc") is None + + +@pytest.mark.parametrize( + "base_rev,base_ref,head_ref,fork,comparisons,expected,bases,default_calls", + [ + ("old", "main", "feature", False, [("ahead", "old")], None, ["old"], 0), + ( + "old", + "main", + "feature", + False, + [("diverged", "merge")], + "merge", + ["old"], + 0, + ), + ( + "old", + "main", + "main", + True, + [("diverged", "merge")], + "merge", + ["old"], + 0, + ), + ("old", "feature", "feature", False, [("ahead", "old")], None, ["old"], 0), + ( + "old", + "refs/heads/feature", + "feature", + False, + [("diverged", "old-base"), ("ahead", "merge")], + "merge", + ["old", "main"], + 1, + ), + ( + "old", + "main", + "refs/heads/main", + False, + [("diverged", "old-base")], + "old-base", + ["old"], + 1, + ), + ( + "0" * 40, + "source", + "feature", + False, + [("ahead", "merge")], + "merge", + ["source"], + 0, + ), + ("0" * 40, None, "feature", False, [("ahead", "merge")], "merge", ["main"], 1), + ("old", "main", "refs/tags/v1", False, [], None, [], 0), + ("old", "feature", "feature", False, [None], None, ["old"], 0), + (None, "main", "feature", False, [None], None, ["main"], 0), + ("old", "main", "feature", False, [("identical", "old")], None, ["old"], 0), + (None, None, "main", False, [], None, [], 1), + ], +) +def test_derive_base_rev( + run_task_mod, + mocker, + base_rev, + base_ref, + head_ref, + fork, + comparisons, + expected, + bases, + default_calls, +): + compare = mocker.patch.object( + run_task_mod, "github_compare", side_effect=comparisons + ) + default = mocker.patch.object(run_task_mod, "_default_branch", return_value="main") + base_repo = "https://github.com/owner/repo" + head_repo = "https://github.com/fork/repo" if fork else base_repo + assert ( + run_task_mod.derive_base_rev( + ("owner", "repo"), + base_repo, + head_repo, + base_rev, + base_ref, + head_ref, + "head", + ) + == expected + ) + assert compare.call_args_list == [ + mocker.call("owner", "repo", base, "head") for base in bases + ] + assert default.call_count == default_calls + + +@pytest.mark.parametrize( + "output,expected", + [("ref: refs/heads/main\tHEAD\nabc\tHEAD\n", "main"), ("abc\tHEAD\n", None)], +) +def test_default_branch(run_task_mod, mocker, output, expected): + check = mocker.patch("subprocess.check_output", return_value=output) + assert run_task_mod._default_branch("https://github.com/owner/repo") == expected + check.assert_called_once_with( + ["git", "ls-remote", "--symref", "https://github.com/owner/repo", "HEAD"], + universal_newlines=True, + timeout=60, + ) + check.side_effect = subprocess.TimeoutExpired("git", 60) + assert run_task_mod._default_branch("https://github.com/owner/repo") is None + + +@pytest.mark.parametrize( + "repo_type,repo,base_rev,base_ref,expected", + [ + ("git", "https://example.com/owner/repo", "old", "main", False), + ("hg", "https://github.com/owner/repo", "old", "main", False), + ("git", "https://github.com/owner/repo", None, None, False), + ("git", "https://github.com/owner/repo", "old", None, True), + ("git", "https://github.com/owner/repo", None, "main", True), + ], +) +def test_vcs_checkout_derive_gating( + run_task_mod, + monkeypatch, + mocker, + tmp_path, + repo_type, + repo, + base_rev, + base_ref, + expected, +): + monkeypatch.setattr(os, "environ", {}) + options = run_task_mod.collect_vcs_options( + Namespace(myrepo_checkout=str(tmp_path), myrepo_shallow_clone=False), + "myrepo", + "myrepo", + ) + options.update( + { + "repo-type": repo_type, + "head-repo": repo, + "head-rev": "head", + "base-rev": base_rev, + "base-ref": base_ref, + } + ) + derive = mocker.patch.object(run_task_mod, "derive_base_rev", return_value=None) + compare = mocker.patch.object(run_task_mod, "github_compare") + mocker.patch.object(run_task_mod, "git_checkout", return_value="head") + mocker.patch.object(run_task_mod, "hg_checkout", return_value="head") + run_task_mod.vcs_checkout_from_args(options) + assert derive.call_count == int(expected) + compare.assert_not_called() + if expected: + assert os.environ["MYREPO_BASE_REV"] == (base_rev or "") + else: + assert "MYREPO_BASE_REV" not in os.environ + + +def test_vcs_checkout_derived_base_fetched( + run_task_mod, monkeypatch, mocker, mock_git_repo, mock_stdin, tmp_path +): + monkeypatch.setenv("GIT_CONFIG_GLOBAL", str(tmp_path / "gitconfig")) + repo_url = "https://github.com/owner/repo" + # Keep the GitHub URL for derivation, but route checkout to the local fixture. + monkeypatch.setenv("GIT_CONFIG_COUNT", "1") + monkeypatch.setenv( + "GIT_CONFIG_KEY_0", f"url.file://{mock_git_repo['path']}.insteadOf" + ) + monkeypatch.setenv("GIT_CONFIG_VALUE_0", repo_url) + monkeypatch.setenv("GIT_ALLOW_PROTOCOL", "file") + derived = mock_git_repo["branch"][-1] + options = run_task_mod.collect_vcs_options( + Namespace( + myrepo_checkout=str(tmp_path / "checkout"), myrepo_shallow_clone=True + ), + "myrepo", + "myrepo", + ) + options.update( + { + "repo-type": "git", + "base-repo": repo_url, + "head-repo": repo_url, + "base-rev": "0" * 40, + "head-rev": mock_git_repo["main"][-1], + "head-ref": "refs/heads/main", + } + ) + derive = mocker.patch.object(run_task_mod, "derive_base_rev", return_value=derived) + run_task_mod.vcs_checkout_from_args(options) + derive.assert_called_once() + assert options["base-rev"] == derived + assert os.environ["MYREPO_BASE_REV"] == derived + assert ( + subprocess.check_output( + ["git", "cat-file", "-t", derived], cwd=options["checkout"], text=True + ).strip() + == "commit" + ) + + +@pytest.mark.parametrize( + "payload", + [ + b"invalid JSON", + b"{}", + b"null", + b'{"status": "ahead", "merge_base_commit": {"sha": "invalid"}}', + ], +) +def test_github_compare_invalid_response(run_task_mod, mocker, payload): + response = mocker.MagicMock() + response.__enter__.return_value.read.return_value = payload + mocker.patch("urllib.request.urlopen", return_value=response) + assert run_task_mod.github_compare("owner", "repo", "main", "head") is None + + +def test_derive_base_rev_default_branch_failure(run_task_mod, mocker): + mocker.patch.object(run_task_mod, "_default_branch", return_value=None) + compare = mocker.patch.object(run_task_mod, "github_compare") + assert ( + run_task_mod.derive_base_rev( + ("owner", "repo"), + "https://github.com/owner/repo", + "https://github.com/owner/repo", + None, + None, + "feature", + "head", + ) + is None + ) + compare.assert_not_called()