From ad795d2c7108a86b3ce0cda6b1784ddcbb9528b8 Mon Sep 17 00:00:00 2001 From: nhadji Date: Thu, 9 Jul 2026 11:25:12 +0200 Subject: [PATCH 1/8] Bug 2048352 - Add a GitRepository to apply and push patch stacks to Git Mirror the Mercurial Repository interface in a new GitRepository class so the analysis workflow can apply a Phabricator patch stack and push it to a Git remote (e.g. a GitHub "try" repository) instead of hg.mozilla.org/try. This is additive: the Mercurial path is unchanged. The repository class to use will be selected per-repository in a later change. Differences from the Mercurial implementation: - the base revision is already a Git hash, so there is no Lando git2hg lookup; - authentication uses an SSH deploy key via GIT_SSH_COMMAND. Add tests (test_git.py) covering patch application, try_task_config.json, push to a local bare repo, base-revision resolution, clean, and the diff-header normalisation. They are self-contained (local git only) and need no Mercurial environment. --- bot/code_review_bot/git.py | 291 +++++++++++++++++++++++++++++++++++++ bot/tests/conftest.py | 57 ++++++++ bot/tests/test_git.py | 157 ++++++++++++++++++++ 3 files changed, 505 insertions(+) create mode 100644 bot/tests/test_git.py diff --git a/bot/code_review_bot/git.py b/bot/code_review_bot/git.py index b5378c775..5e2a526c9 100644 --- a/bot/code_review_bot/git.py +++ b/bot/code_review_bot/git.py @@ -1,10 +1,31 @@ +import atexit +import json +import os +import re +import tempfile from urllib.parse import urlparse import structlog from git import Repo +from git.exc import GitCommandError +from libmozdata.phabricator import PhabricatorPatch + +from code_review_bot.sources.phabricator import PhabricatorBuild logger = structlog.getLogger(__name__) +# Default author for commits without explicit Phabricator author data, and the +# committer for all bot-created commits. Matches the Mercurial worker. +DEFAULT_AUTHOR_NAME = "code review bot" +DEFAULT_AUTHOR_EMAIL = "release-mgmt-analysis@mozilla.com" + +# Matches a trailing "Weekday Mon DD HH:MM:SS YYYY +ZZZZ" timestamp that some +# Phabricator/Mercurial raw diffs append to the ---/+++ header lines. `git apply` +# would treat it as part of the filename, so it is stripped before applying. +DIFF_HEADER_TIMESTAMP = re.compile( + r"[ \t]+\w{3}\s+\w{3}\s+\d{1,2}\s+\d{2}:\d{2}:\d{2}\s+\d{4}\s+[+-]\d{4}\s*$" +) + def build_repo_slug(repo_url): """ @@ -72,3 +93,273 @@ def git_clone(base_repository, head_repository, revision, destination): repo.head.reference = repo.commit(revision) return repo + + +class GitRepository: + """ + A Git repository with credentials to push a patch stack to a remote + (e.g. a GitHub "try" repository). + + Mirrors the interface of the Mercurial ``Repository`` + (``code_review_bot.mercurial.Repository``) so the same workflow can apply a + Phabricator patch stack and push it, but targeting Git instead of Mercurial. + + Notable differences from the Mercurial implementation: + - the base revision is already a Git hash, so there is no Lando ``git2hg`` lookup; + - authentication uses an SSH deploy key passed through ``GIT_SSH_COMMAND``. + """ + + def __init__(self, config, cache_root): + assert isinstance(config, dict) + self.name = config["name"] + self.url = config["url"] + self.dir = os.path.join(cache_root, config["name"]) + self.try_url = config["try_url"] + self.try_name = config.get("try_name", "try") + + # Revision/branch to apply patches on when the base is unknown locally + self.default_revision = config.get("default_revision", "HEAD") + + # Branch pushed to the remote try repository. + # TODO: the per-build ref model is still an open question (see plan Q2). + self.head_branch = config.get("head_branch", "code-review") + + # Apply patches on the latest revision when True + self.use_latest_revision = config.get("use_latest_revision", False) + + self._repo = None + + # Write the SSH (deploy) key from the configuration to a temporary file + _, self.ssh_key_path = tempfile.mkstemp(suffix=".key") + with open(self.ssh_key_path, "w") as f: + f.write(config["ssh_key"]) + os.chmod(self.ssh_key_path, 0o600) + self.git_ssh_command = f"ssh -i {self.ssh_key_path} -o StrictHostKeyChecking=no" + + # Remove the key when finished + atexit.register(self.end_of_life) + + def __str__(self): + return self.name + + def end_of_life(self): + if os.path.exists(self.ssh_key_path): + os.unlink(self.ssh_key_path) + logger.info("Removed ssh key") + + @property + def repo(self): + """Lazily open the local Git repository.""" + if self._repo is None: + logger.info(f"Git open {self.dir}") + self._repo = Repo(self.dir) + return self._repo + + def clone(self): + logger.info("Checking out git repository", repo=self.url, dir=self.dir) + if os.path.isdir(os.path.join(self.dir, ".git")): + self._repo = Repo(self.dir) + with self.repo.git.custom_environment(GIT_SSH_COMMAND=self.git_ssh_command): + self.repo.remotes.origin.fetch() + else: + self._repo = Repo.clone_from( + self.url, self.dir, env={"GIT_SSH_COMMAND": self.git_ssh_command} + ) + logger.info("Full checkout finished") + + def has_revision(self, revision): + """Check whether a revision exists in the local Git repository.""" + if not revision: + return False + try: + self.repo.git.cat_file("-e", f"{revision}^{{commit}}") + return True + except GitCommandError: + return False + + def get_base_identifier(self, needed_stack: list[PhabricatorPatch]) -> str: + """Return the base identifier to apply patches against. + + Unlike Mercurial, the base revision is already a Git hash, so there is no + Lando ``git2hg`` conversion: when the base is not available locally we fall + back to the default revision. + """ + if self.use_latest_revision: + return self.default_revision + + base_revision = needed_stack[0].base_revision + if self.has_revision(base_revision): + return base_revision + + logger.warning( + "Base revision not available locally, using the default revision", + revision=base_revision, + default=self.default_revision, + ) + return self.default_revision + + @staticmethod + def get_author(commit): + """Build a ``(name, email)`` tuple from Phabricator commit data.""" + author = commit.get("author") if commit else None + if author is None: + return DEFAULT_AUTHOR_NAME, DEFAULT_AUTHOR_EMAIL + if author.get("name") and author.get("email"): + return author["name"], author["email"] + # Fall back to parsing the raw "Name " representation + raw = author.get("raw", "") or "" + match = re.match(r"^(?P.*?)\s*<(?P.*)>\s*$", raw) + if match: + return match.group("name"), match.group("email") + return (raw or DEFAULT_AUTHOR_NAME), DEFAULT_AUTHOR_EMAIL + + @staticmethod + def normalize_patch(patch: str) -> str: + """Strip trailing timestamps from ---/+++ header lines. + + Some Phabricator/Mercurial raw diffs append a "Weekday Mon DD ..." timestamp + to the header filenames; ``git apply`` would treat it as part of the filename. + """ + lines = [] + for line in patch.splitlines(): + if line.startswith("--- ") or line.startswith("+++ "): + line = DIFF_HEADER_TIMESTAMP.sub("", line) + lines.append(line) + return "\n".join(lines) + "\n" + + def commit_patch(self, patch_content, message, name, email): + """Apply a single unified diff to the index and commit it.""" + with tempfile.NamedTemporaryFile( + mode="w", suffix=".diff", delete=False + ) as patch_file: + patch_file.write(self.normalize_patch(patch_content)) + patch_path = patch_file.name + + try: + self.repo.git.apply("--index", patch_path) + env = { + "GIT_AUTHOR_NAME": name, + "GIT_AUTHOR_EMAIL": email, + "GIT_COMMITTER_NAME": DEFAULT_AUTHOR_NAME, + "GIT_COMMITTER_EMAIL": DEFAULT_AUTHOR_EMAIL, + } + with self.repo.git.custom_environment(**env): + self.repo.git.commit("--no-verify", "-m", message) + finally: + os.unlink(patch_path) + + def apply_build(self, build): + """Apply a stack of Phabricator patches as Git commits.""" + assert isinstance(build, PhabricatorBuild) + assert len(build.stack) > 0, "No patches to apply" + assert all(isinstance(p, PhabricatorPatch) for p in build.stack) + + # Find the first unknown base revision + needed_stack = [] + for patch in reversed(build.stack): + # Skip already merged patches + if patch.merged: + logger.info( + f"Skip applying patch {patch.id} as it's already been merged upstream" + ) + continue + + # Add the patch into the stack only if not already merged + needed_stack.insert(0, patch) + + # Stop as soon as a base revision is available + if self.has_revision(patch.base_revision): + logger.info(f"Stopping at revision {patch.base_revision}") + break + + if not needed_stack: + logger.info("All the patches are already applied") + return + + git_base = self.get_base_identifier(needed_stack) + + # When base revision is missing, fall back to the default revision + build.base_revision = git_base + build.missing_base_revision = not self.has_revision(git_base) + if build.missing_base_revision: + logger.warning( + "Missing base revision from Phabricator", + revision=git_base, + fallback=self.default_revision, + ) + git_base = self.default_revision + + # Store the actual base revision we used + build.actual_base_revision = git_base + + # Move the working tree to the base revision + logger.info(f"Updating repo to revision {git_base}") + self.repo.git.checkout(git_base, force=True) + + for patch in needed_stack: + if patch.commits: + # Use the first commit only + commit = patch.commits[0] + message = "{}\n".format(commit["message"]) + name, email = self.get_author(commit) + else: + # We should always have some commits here + logger.warning("Missing commit on patch", id=patch.id) + message = "" + name, email = DEFAULT_AUTHOR_NAME, DEFAULT_AUTHOR_EMAIL + message += f"Differential Diff: {patch.phid}" + + logger.info("Applying patch", phid=patch.phid, message=message) + self.commit_patch(patch.patch, message, name, email) + + def add_try_commit(self, build): + """ + Build and commit the file configuring try with try_task_config.json + and the code-review workflow parameters in JSON + """ + path = os.path.join(self.dir, "try_task_config.json") + config = { + "version": 2, + "parameters": { + "target_tasks_method": "codereview", + "optimize_target_tasks": True, + "phabricator_diff": build.target_phid, + }, + } + diff_phid = build.stack[-1].phid + + if build.revision_url: + message = f"try_task_config for {build.revision_url}" + else: + message = "try_task_config for code-review" + message += f"\nDifferential Diff: {diff_phid}" + + # Write content as json and commit it + with open(path, "w") as f: + json.dump(config, f, sort_keys=True, indent=4) + + self.repo.git.add(path) + env = { + "GIT_AUTHOR_NAME": DEFAULT_AUTHOR_NAME, + "GIT_AUTHOR_EMAIL": DEFAULT_AUTHOR_EMAIL, + "GIT_COMMITTER_NAME": DEFAULT_AUTHOR_NAME, + "GIT_COMMITTER_EMAIL": DEFAULT_AUTHOR_EMAIL, + } + with self.repo.git.custom_environment(**env): + self.repo.git.commit("--no-verify", "-m", message) + + def push_to_try(self): + """Push the current HEAD to the remote try repository over the deploy key.""" + head = self.repo.head.commit + logger.info("Pushing patches to try", rev=head.hexsha, branch=self.head_branch) + with self.repo.git.custom_environment(GIT_SSH_COMMAND=self.git_ssh_command): + self.repo.git.push( + self.try_url, f"HEAD:refs/heads/{self.head_branch}", force=True + ) + return head + + def clean(self): + """Reset the local checkout to a pristine state.""" + logger.info("Cleaning git checkout") + self.repo.git.reset("--hard") + self.repo.git.clean("-fxd") diff --git a/bot/tests/conftest.py b/bot/tests/conftest.py index 56bf2d1ff..f1448becf 100644 --- a/bot/tests/conftest.py +++ b/bot/tests/conftest.py @@ -1191,6 +1191,63 @@ def mock_nss(tmpdir): return repo +def build_git_repository(tmpdir, name): + """ + Mock a local git repo with a single base commit (no remote access). + Mirror of build_repository() for the Mercurial tests. + """ + from git import Actor, Repo + + repo_dir = str(tmpdir.mkdir(name).realpath()) + repo = Repo.init(repo_dir) + + # Set a committer identity and disable signing for the fixture + with repo.config_writer() as cw: + cw.set_value("user", "name", "test") + cw.set_value("user", "email", "test") + cw.set_value("commit", "gpgsign", "false") + + # Commit a Readme as the base revision + readme = os.path.join(repo_dir, "README.md") + with open(readme, "w") as f: + f.write("Hello World") + repo.index.add(["README.md"]) + actor = Actor("test", "test") + repo.index.commit("Readme", author=actor, committer=actor) + + return repo + + +@pytest.fixture +def mock_mc_git(tmpdir): + """ + Mock a Mozilla Central repository backed by Git + """ + from git import Repo + + from code_review_bot.git import GitRepository + + repo = build_git_repository(tmpdir, "mozilla-central") + + # A local bare repo acts as the remote "try" target (no network access) + try_dir = str(tmpdir.mkdir("try.git").realpath()) + Repo.init(try_dir, bare=True) + + config = { + "name": "mozilla-central", + "url": "https://github.com/mozilla/test", + "try_url": try_dir, + "try_name": "try", + "ssh_key": "privateSSHkey", + "default_revision": repo.active_branch.name, + "head_branch": "code-review", + } + git_repo = GitRepository(config, str(tmpdir.realpath())) + git_repo._repo = repo + git_repo.clone = MagicMock(side_effect=lambda: True) + return git_repo + + @pytest.fixture @contextmanager def PhabricatorMock(): diff --git a/bot/tests/test_git.py b/bot/tests/test_git.py new file mode 100644 index 000000000..cc4050fd0 --- /dev/null +++ b/bot/tests/test_git.py @@ -0,0 +1,157 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. +import json +import os.path +from unittest.mock import MagicMock + +from conftest import MockBuild + +from code_review_bot.git import GitRepository + +# A diff whose base revision exists neither in Git nor Mercurial: patches will be +# applied on the repository's default revision (mirrors the Mercurial tests). +DIFF = { + "phid": "PHID-DIFF-test123", + "revisionPHID": "PHID-DREV-deadbeef", + "id": 1234, + "baseRevision": "abcdef123456", +} + + +def make_build(phabricator_mock): + """Build a MockBuild with its patch stack loaded from the Phabricator mock.""" + build = MockBuild(1234, "PHID-REPO-mc", 5678, "PHID-HMBT-deadbeef", dict(DIFF)) + with phabricator_mock as phab: + phab.load_patches_stack(build) + return build + + +def test_normalize_patch(): + """Trailing Mercurial-style timestamps are stripped from ---/+++ headers.""" + patch = ( + "diff -r 000000000000 test.txt\n" + "--- /dev/null Thu Jan 01 00:00:00 1970 +0000\n" + "+++ b/test.txt Tue Feb 05 17:23:40 2019 +0100\n" + "@@ -0,0 +1,1 @@\n" + "+First Line\n" + ) + normalized = GitRepository.normalize_patch(patch) + assert "--- /dev/null\n" in normalized + assert "+++ b/test.txt\n" in normalized + assert "1970" not in normalized and "2019" not in normalized + + # Git-style headers (no timestamp) are left untouched + git_patch = "--- a/x\n+++ b/x\n@@ -1 +1 @@\n-a\n+b\n" + assert GitRepository.normalize_patch(git_patch) == git_patch + + +def test_has_revision(mock_mc_git): + head = mock_mc_git.repo.head.commit.hexsha + assert mock_mc_git.has_revision(head) is True + assert mock_mc_git.has_revision(head[:12]) is True + assert mock_mc_git.has_revision("deadbeef" * 5) is False + assert mock_mc_git.has_revision("") is False + assert mock_mc_git.has_revision(None) is False + + +def test_get_base_identifier_no_git2hg(mock_mc_git): + """The git base is used directly: no Lando git2hg lookup, fall back to default.""" + head = mock_mc_git.repo.head.commit.hexsha + + # Base present locally -> returned as-is + present = MagicMock(base_revision=head) + assert mock_mc_git.get_base_identifier([present]) == head + + # Base absent -> fall back to the default revision (no git2hg call) + absent = MagicMock(base_revision="abcdef123456") + assert mock_mc_git.get_base_identifier([absent]) == mock_mc_git.default_revision + + +def test_apply_patches(PhabricatorMock, mock_mc_git): + """Apply a Phabricator stack as Git commits onto the default revision.""" + build = make_build(PhabricatorMock) + + target = os.path.join(mock_mc_git.dir, "test.txt") + assert not os.path.exists(target) + + mock_mc_git.apply_build(build) + + # The patched file now has the expected content + assert os.path.exists(target) + assert open(target).read() == "First Line\nSecond Line\n" + + # Commits (newest first): the two patches on top of the base Readme + commits = list(mock_mc_git.repo.iter_commits()) + assert [c.message.strip() for c in commits] == [ + "Bug XXX - A second commit message\nDifferential Diff: PHID-DIFF-test123", + "Bug XXX - A first commit message\nDifferential Diff: PHID-DIFF-xxxx", + "Readme", + ] + assert [f"{c.author.name} <{c.author.email}>" for c in commits] == [ + "John Doe ", + "randomUsername ", + "test ", + ] + + +def test_add_try_commit(PhabricatorMock, mock_mc_git): + """try_task_config.json is written and committed by the bot author.""" + build = make_build(PhabricatorMock) + mock_mc_git.apply_build(build) + mock_mc_git.add_try_commit(build) + + config_path = os.path.join(mock_mc_git.dir, "try_task_config.json") + assert os.path.exists(config_path) + assert json.load(open(config_path)) == { + "version": 2, + "parameters": { + "target_tasks_method": "codereview", + "optimize_target_tasks": True, + "phabricator_diff": "PHID-HMBT-deadbeef", + }, + } + + head = next(mock_mc_git.repo.iter_commits()) + assert ( + head.message.strip() + == "try_task_config for code-review\nDifferential Diff: PHID-DIFF-test123" + ) + assert ( + f"{head.author.name} <{head.author.email}>" + == "code review bot " + ) + + +def test_push_to_try(PhabricatorMock, mock_mc_git): + """push_to_try pushes the prepared HEAD to the configured branch/remote.""" + build = make_build(PhabricatorMock) + mock_mc_git.apply_build(build) + mock_mc_git.add_try_commit(build) + + pushed = mock_mc_git.push_to_try() + + assert pushed == mock_mc_git.repo.head.commit + + # The remote try repo received the configured branch at the pushed commit + from git import Repo + + remote = Repo(mock_mc_git.try_url) + assert remote.refs["code-review"].commit.hexsha == pushed.hexsha + + +def test_clean(mock_mc_git): + """clean() resets tracked changes and removes untracked files.""" + untracked = os.path.join(mock_mc_git.dir, "untracked.txt") + with open(untracked, "w") as f: + f.write("dirty") + readme = os.path.join(mock_mc_git.dir, "README.md") + with open(readme, "w") as f: + f.write("changed") + assert mock_mc_git.repo.is_dirty(untracked_files=True) + + mock_mc_git.clean() + + assert not mock_mc_git.repo.is_dirty(untracked_files=True) + assert not os.path.exists(untracked) + assert open(readme).read() == "Hello World" From 9e3b259b48543eceaf9a3f433f467a7a368b6d43 Mon Sep 17 00:00:00 2001 From: nhadji Date: Thu, 9 Jul 2026 11:25:12 +0200 Subject: [PATCH 2/8] Bug 2048352 - Add a GitWorker and make GitRepository.clean() pristine Add a GitWorker mirroring MercurialWorker for Git: clean the clone, apply the Phabricator patch stack, write try_task_config.json, push to the remote try repository and return a Treeherder link. Unlike the Mercurial worker there is no treestatus wait (Git has no "try" tree); eligible push errors are retried with exponential backoff. Also make clean() equivalent to the Mercurial one. apply_build now checks out the base with a detached HEAD so the bot's patch and try_task_config commits stay throwaway drafts, and clean() refreshes from the remote (when configured) and resets back to the base. This prevents a reused cached clone from accumulating commits across builds. Add tests for the worker success path, skippable patches, general failures, retry-without-treestatus, and that a reused clone drops the previous build's commits. --- bot/code_review_bot/git.py | 201 ++++++++++++++++++++++++++++++++++++- bot/tests/test_git.py | 111 +++++++++++++++++++- 2 files changed, 308 insertions(+), 4 deletions(-) diff --git a/bot/code_review_bot/git.py b/bot/code_review_bot/git.py index 5e2a526c9..235c9e0e7 100644 --- a/bot/code_review_bot/git.py +++ b/bot/code_review_bot/git.py @@ -3,8 +3,10 @@ import os import re import tempfile +import time from urllib.parse import urlparse +import rs_parsepatch import structlog from git import Repo from git.exc import GitCommandError @@ -14,6 +16,16 @@ logger = structlog.getLogger(__name__) +# Treeherder job URL for a pushed revision. The repository name (first slot) is +# the Treeherder repo, refined for the Git try repository in a later change. +TREEHERDER_URL = "https://treeherder.mozilla.org/#/jobs?repo={}&revision={}" + +# Number of allowed retries on an unexpected push failure, and the base of the +# exponential backoff between them (6s, 36s, 3.6min, 21.6min). Mirrors the +# Mercurial worker, minus the treestatus wait (there is no Git "try" tree). +MAX_PUSH_RETRIES = 4 +PUSH_RETRY_EXPONENTIAL_DELAY = 6 + # Default author for commits without explicit Phabricator author data, and the # committer for all bot-created commits. Matches the Mercurial worker. DEFAULT_AUTHOR_NAME = "code review bot" @@ -27,6 +39,12 @@ ) +class RetryNeeded(Exception): + """ + Raised when retrying a Git build is needed + """ + + def build_repo_slug(repo_url): """ Build a slug from a github repository url @@ -292,9 +310,11 @@ def apply_build(self, build): # Store the actual base revision we used build.actual_base_revision = git_base - # Move the working tree to the base revision + # Move the working tree to the base revision. Detach HEAD so the patches + # we commit on top stay throwaway drafts and never advance a branch ref; + # clean() can then discard them simply by returning to the base. logger.info(f"Updating repo to revision {git_base}") - self.repo.git.checkout(git_base, force=True) + self.repo.git.checkout(git_base, force=True, detach=True) for patch in needed_stack: if patch.commits: @@ -359,7 +379,182 @@ def push_to_try(self): return head def clean(self): - """Reset the local checkout to a pristine state.""" + """Reset the local checkout to a pristine state. + + Mirrors the Mercurial ``clean()`` (revert + strip outgoing drafts + + pull): a reused clone can hold the patch commits and ``try_task_config`` + commit from a previous build, so we discard local changes, refresh from + the remote and return to the base revision. Because ``apply_build`` + commits on a detached HEAD, returning to the base leaves those commits + unreferenced instead of accumulating them on a branch. + """ logger.info("Cleaning git checkout") + + # Discard uncommitted changes and untracked/ignored files self.repo.git.reset("--hard") self.repo.git.clean("-fxd") + + # Refresh from the remote when one is configured (mirrors hg pull) + if any(remote.name == "origin" for remote in self.repo.remotes): + with self.repo.git.custom_environment(GIT_SSH_COMMAND=self.git_ssh_command): + self.repo.remotes.origin.fetch() + + # Return to the pristine base, dropping any previously applied commits. + # Prefer the remote-tracking base so we also pick up upstream updates. + upstream = f"origin/{self.default_revision}" + target = upstream if self.has_revision(upstream) else self.default_revision + self.repo.git.checkout(self.default_revision, force=True) + self.repo.git.reset("--hard", target) + + +class GitWorker: + """ + Drive a GitRepository through a single build: clean, apply the patch stack, + push to the remote try repository and return a Treeherder link. + + Mirrors ``code_review_bot.mercurial.MercurialWorker`` but for Git. The key + difference is that there is no treestatus wait before retrying: Git has no + "try" tree to gate on, so failed pushes are simply retried with backoff. + """ + + ELIGIBLE_RETRY_ERRORS = [ + error.lower() + for error in [ + "could not read from remote repository", + "connection closed by remote host", + "connection timed out", + "early eof", + "rpc failed", + "the remote end hung up unexpectedly", + "ssh_exchange_identification", + ] + ] + + def __init__(self, skippable_files=[]): + self.skippable_files = skippable_files + + def run(self, repository, build): + """ + Apply the stack of patches from the build, retrying on remote push + errors. Unlike the Mercurial worker, there is no treestatus wait. + """ + while build.retries <= MAX_PUSH_RETRIES: + start = time.time() + + if build.retries: + logger.warning( + "Trying to apply build's diff after a remote push error " + f"[{build.retries}/{MAX_PUSH_RETRIES}]" + ) + + try: + return self.handle_build(repository, build) + except RetryNeeded: + build.retries += 1 + + if build.retries > MAX_PUSH_RETRIES: + error_log = "Max number of retries has been reached pushing the build to try repository" + logger.warn("Git error on diff", error=error_log, build=build) + return ( + "fail:git", + build, + {"message": error_log, "duration": time.time() - start}, + ) + + # Wait an exponential time before retrying the build + delay = PUSH_RETRY_EXPONENTIAL_DELAY**build.retries + logger.info( + f"An error occurred pushing the build to try, retrying after {delay}s" + ) + time.sleep(delay) + + def is_commit_skippable(self, build): + def get_files_touched_in_diff(rawdiff): + patched = [] + for parsed_diff in rs_parsepatch.get_diffs(rawdiff): + # filename is sometimes of format 'test.txt Tue Feb 05 17:23:40 2019 +0100' + # fix after https://github.com/mozilla/rust-parsepatch/issues/61 + if "filename" in parsed_diff: + filename = parsed_diff["filename"].split(" ")[0] + patched.append(filename) + return patched + + return any( + patched_file in self.skippable_files + for rev in build.stack + for patched_file in get_files_touched_in_diff(rev.patch) + ) + + def is_eligible_for_retry(self, error): + """ + Given a Git error message, if it's likely due to a temporary connection + problem, consider it eligible for retry. + """ + error = error.lower() + return any( + eligible_message in error for eligible_message in self.ELIGIBLE_RETRY_ERRORS + ) + + def handle_build(self, repository, build): + """ + Apply the build's diff on the local clone; on success push to try and + return a treeherder link. Unexpected push failures raise RetryNeeded so + run() retries; other failures return a warning result. + """ + assert isinstance(repository, GitRepository) + start = time.time() + + try: + # Start by cleaning the repo + repository.clean() + + # First apply patches on local repo + repository.apply_build(build) + + # Check Eligibility: some commits don't need to be pushed to try. + if self.is_commit_skippable(build): + logger.info("This patch series is ineligible for automated try push") + return ( + "fail:ineligible", + build, + { + "message": "Modified files match skippable internal configuration files", + "duration": time.time() - start, + }, + ) + + # Configure the try task + repository.add_try_commit(build) + + # Then push that stack on try + tip = repository.push_to_try() + logger.info("Diff has been pushed !") + + # Publish Treeherder link + uri = TREEHERDER_URL.format(repository.try_name, tip.hexsha) + except GitCommandError as e: + error_log = e.stderr or str(e) + + if self.is_eligible_for_retry(error_log): + raise RetryNeeded + + logger.warn("Git error on diff", error=error_log, args=e.args, build=build) + return ( + "fail:git", + build, + {"message": error_log, "duration": time.time() - start}, + ) + + except Exception as e: + logger.warn("Failed to process diff", error=e, build=build) + return ( + "fail:general", + build, + {"message": str(e), "duration": time.time() - start}, + ) + + return ( + "success", + build, + {"treeherder_url": uri, "revision": tip.hexsha}, + ) diff --git a/bot/tests/test_git.py b/bot/tests/test_git.py index cc4050fd0..55f46c7f1 100644 --- a/bot/tests/test_git.py +++ b/bot/tests/test_git.py @@ -6,8 +6,9 @@ from unittest.mock import MagicMock from conftest import MockBuild +from git.exc import GitCommandError -from code_review_bot.git import GitRepository +from code_review_bot.git import MAX_PUSH_RETRIES, GitRepository, GitWorker # A diff whose base revision exists neither in Git nor Mercurial: patches will be # applied on the repository's default revision (mirrors the Mercurial tests). @@ -155,3 +156,111 @@ def test_clean(mock_mc_git): assert not mock_mc_git.repo.is_dirty(untracked_files=True) assert not os.path.exists(untracked) assert open(readme).read() == "Hello World" + + +def test_clean_drops_previous_build(PhabricatorMock, mock_mc_git): + """A reused clone does not accumulate a previous build's commits.""" + branch = mock_mc_git.default_revision + base = mock_mc_git.repo.commit(branch).hexsha + assert mock_mc_git.repo.git.rev_list("--count", branch).strip() == "1" + + # First build: apply the stack and the try_task_config commit + build = make_build(PhabricatorMock) + mock_mc_git.apply_build(build) + mock_mc_git.add_try_commit(build) + + # Those commits live on a detached HEAD; the branch has not moved + assert mock_mc_git.repo.head.is_detached + assert mock_mc_git.repo.commit(branch).hexsha == base + assert os.path.exists(os.path.join(mock_mc_git.dir, "test.txt")) + + # Cleaning returns to the pristine base, dropping the build's commits + mock_mc_git.clean() + + assert mock_mc_git.repo.head.commit.hexsha == base + assert mock_mc_git.repo.git.rev_list("--count", branch).strip() == "1" + assert not os.path.exists(os.path.join(mock_mc_git.dir, "test.txt")) + assert not os.path.exists(os.path.join(mock_mc_git.dir, "try_task_config.json")) + + +def test_worker_run_success(PhabricatorMock, mock_mc_git): + """Full success path: apply, configure try, push, return treeherder link.""" + build = make_build(PhabricatorMock) + + worker = GitWorker() + result = worker.run(mock_mc_git, build) + + tip = mock_mc_git.repo.head.commit + assert result == ( + "success", + build, + { + "revision": tip.hexsha, + "treeherder_url": ( + "https://treeherder.mozilla.org/#/jobs?repo=try&revision=" + f"{tip.hexsha}" + ), + }, + ) + + # The remote try repo received the configured branch at the pushed commit + from git import Repo + + remote = Repo(mock_mc_git.try_url) + assert remote.refs["code-review"].commit.hexsha == tip.hexsha + + +def test_worker_skippable(PhabricatorMock, mock_mc_git): + """A patch touching only skippable files is not pushed to try.""" + build = make_build(PhabricatorMock) + + worker = GitWorker(skippable_files=["test.txt"]) + mode, out_build, details = worker.run(mock_mc_git, build) + + assert mode == "fail:ineligible" + assert out_build is build + assert "skippable" in details["message"] + # Nothing was pushed + from git import Repo + + assert "code-review" not in Repo(mock_mc_git.try_url).refs + + +def test_worker_failure_general(PhabricatorMock, mock_mc_git): + """A non-Git error while applying yields a fail:general result.""" + build = make_build(PhabricatorMock) + mock_mc_git.apply_build = MagicMock(side_effect=Exception("boom")) + + worker = GitWorker() + mode, out_build, details = worker.run(mock_mc_git, build) + + assert mode == "fail:general" + assert details["message"] == "boom" + + +def test_worker_retry_no_treestatus(PhabricatorMock, mock_mc_git, monkeypatch): + """Eligible push errors are retried (no treestatus wait) up to the max.""" + build = make_build(PhabricatorMock) + + # Isolate the worker's retry logic from the repo mechanics + mock_mc_git.clean = MagicMock() + mock_mc_git.apply_build = MagicMock() + mock_mc_git.add_try_commit = MagicMock() + error = GitCommandError( + "git push", 128, b"fatal: Could not read from remote repository" + ) + mock_mc_git.push_to_try = MagicMock(side_effect=error) + + # Don't actually wait through the exponential backoff + monkeypatch.setattr("code_review_bot.git.time.sleep", lambda *a, **k: None) + + worker = GitWorker() + + # The Git worker has no treestatus gate at all + assert not hasattr(worker, "wait_try_available") + + mode, out_build, details = worker.run(mock_mc_git, build) + + assert mode == "fail:git" + # Initial attempt + one per retry + assert mock_mc_git.push_to_try.call_count == MAX_PUSH_RETRIES + 1 From b0e5ba3765177061511663c722edcd1b9af1a54f Mon Sep 17 00:00:00 2001 From: nhadji Date: Thu, 9 Jul 2026 11:26:04 +0200 Subject: [PATCH 3/8] Bug 2048352 - Select the Git or Mercurial backend per repository Add an optional repo_type field to RepositoryConf (default "hg" so existing repository secrets keep working) and branch start_analysis on it: a repository with repo_type "git" builds a GitRepository + GitWorker using the Git cache, while everything else keeps the Mercurial Repository + MercurialWorker. Add tests: repo_type defaults to hg and accepts git, and start_analysis picks the right backend for each repo type. --- bot/code_review_bot/config.py | 22 +++++--- bot/code_review_bot/workflow.py | 50 +++++++++++------ bot/tests/test_phabricator_analysis.py | 75 ++++++++++++++++++++++++++ 3 files changed, 123 insertions(+), 24 deletions(-) diff --git a/bot/code_review_bot/config.py b/bot/code_review_bot/config.py index 1fc398c9a..2a44a1c80 100644 --- a/bot/code_review_bot/config.py +++ b/bot/code_review_bot/config.py @@ -24,7 +24,10 @@ ) RepositoryConf = collections.namedtuple( "RepositoryConf", - "name, try_name, url, try_url, decision_env_prefix, ssh_user", + "name, try_name, url, try_url, decision_env_prefix, ssh_user, repo_type", + # repo_type is optional and defaults to Mercurial so existing repository + # secrets keep working; set it to "git" to push to a Git remote instead. + defaults=("hg",), ) @@ -123,13 +126,18 @@ def build_conf(nb, repo): assert isinstance( repo, dict ), "Repository configuration #{nb+1} is not a dict" - data = [] + data = {} for key in RepositoryConf._fields: - assert ( - key in repo - ), f"Missing key {key} in repository configuration #{nb+1}" - data.append(repo[key]) - return RepositoryConf._make(data) + if key in repo: + data[key] = repo[key] + elif key in RepositoryConf._field_defaults: + # Optional field, fall back to its default (e.g. repo_type) + data[key] = RepositoryConf._field_defaults[key] + else: + raise AssertionError( + f"Missing key {key} in repository configuration #{nb+1}" + ) + return RepositoryConf(**data) self.repositories = [build_conf(i, repo) for i, repo in enumerate(repositories)] assert self.repositories, "No repositories available" diff --git a/bot/code_review_bot/workflow.py b/bot/code_review_bot/workflow.py index 2655f6fd0..92b0afa56 100644 --- a/bot/code_review_bot/workflow.py +++ b/bot/code_review_bot/workflow.py @@ -18,7 +18,7 @@ ) from code_review_bot.backend import BackendAPI from code_review_bot.config import settings -from code_review_bot.git import git_clone +from code_review_bot.git import GitRepository, GitWorker, git_clone from code_review_bot.mercurial import MercurialWorker, Repository, robust_checkout from code_review_bot.report.debug import DebugReporter from code_review_bot.revisions import GithubRevision, PhabricatorRevision, Revision @@ -295,21 +295,38 @@ def start_analysis(self, revision): api_key=self.phabricator.api_key, ) - # Initialize mercurial repository - repository = Repository( - config={ - "name": revision.base_repository_conf.name, - "try_name": revision.base_repository_conf.try_name, - "url": revision.base_repository_conf.url, - "try_url": revision.base_repository_conf.try_url, - # Setup ssh identity - "ssh_user": revision.base_repository_conf.ssh_user, - "ssh_key": settings.ssh_key, - # Force usage of robustcheckout - "checkout": "robust", - }, - cache_root=settings.mercurial_cache, - ) + # Initialize the repository and worker for the configured backend. + # Git is selected per-repository via repo_type; Mercurial is the default. + base_conf = revision.base_repository_conf + if base_conf.repo_type == "git": + repository = GitRepository( + config={ + "name": base_conf.name, + "try_name": base_conf.try_name, + "url": base_conf.url, + "try_url": base_conf.try_url, + # Setup ssh identity (a GitHub deploy key) + "ssh_key": settings.ssh_key, + }, + cache_root=settings.git_cache, + ) + worker = GitWorker() + else: + repository = Repository( + config={ + "name": base_conf.name, + "try_name": base_conf.try_name, + "url": base_conf.url, + "try_url": base_conf.try_url, + # Setup ssh identity + "ssh_user": base_conf.ssh_user, + "ssh_key": settings.ssh_key, + # Force usage of robustcheckout + "checkout": "robust", + }, + cache_root=settings.mercurial_cache, + ) + worker = MercurialWorker() # Try to update the state 5 consecutive time for i in range(5): @@ -340,7 +357,6 @@ def start_analysis(self, revision): repository.clone() # Apply the stack of patches and push to try - worker = MercurialWorker() output = worker.run(repository, build) # Update index when the patch has been pushed to try diff --git a/bot/tests/test_phabricator_analysis.py b/bot/tests/test_phabricator_analysis.py index 5e64eec6c..31d74b0c4 100644 --- a/bot/tests/test_phabricator_analysis.py +++ b/bot/tests/test_phabricator_analysis.py @@ -5,11 +5,13 @@ import json import tempfile from unittest import mock +from unittest.mock import MagicMock import pytest from libmozdata.phabricator import ConduitError from code_review_bot import mercurial +from code_review_bot import workflow as workflow_module from code_review_bot.analysis import ( publish_analysis_phabricator, ) @@ -270,3 +272,76 @@ def test_publish_analysis_phabricator_reraises_other_conduit_errors(): payload = ("success", build, {"treeherder_url": "https://treeherder.mozilla.org/"}) with pytest.raises(ConduitError): publish_analysis_phabricator(payload, phabricator_api) + + +def test_repository_conf_repo_type(): + """repo_type is optional, defaults to hg, and can be set to git (additive).""" + conf = RepositoryConf( + name="mozilla-central", + try_name="try", + url="https://hg.mozilla.org/mozilla-central", + try_url="ssh://hg.mozilla.org/try", + decision_env_prefix="GECKO", + ssh_user="reviewbot@mozilla.com", + ) + assert conf.repo_type == "hg" + assert conf._replace(repo_type="git").repo_type == "git" + + +@pytest.mark.parametrize("repo_type, uses_git", [("git", True), ("hg", False)]) +def test_start_analysis_selects_backend( + mock_phabricator, + mock_workflow, + mock_config, + tmpdir, + monkeypatch, + repo_type, + uses_git, +): + """start_analysis picks the Git or Mercurial backend from repo_type.""" + mock_config.mercurial_cache = tmpdir + mock_config.git_cache = tmpdir + mock_config.ssh_key = "Dummy Private SSH Key" + + # Force the configured repository's backend type + mock_config.repositories = [ + conf._replace(repo_type=repo_type) for conf in mock_config.repositories + ] + + # Build never expires so the analysis proceeds + monkeypatch.setattr(PhabricatorActions, "is_expired_build", lambda _, build: False) + + # Replace both backends with mocks so nothing clones or pushes for real + git_repo, git_worker = MagicMock(), MagicMock() + hg_repo, hg_worker = MagicMock(), MagicMock() + monkeypatch.setattr(workflow_module, "GitRepository", git_repo) + monkeypatch.setattr(workflow_module, "GitWorker", git_worker) + monkeypatch.setattr(workflow_module, "Repository", hg_repo) + monkeypatch.setattr(workflow_module, "MercurialWorker", hg_worker) + git_worker.return_value.run.return_value = ("success", MagicMock(), {}) + hg_worker.return_value.run.return_value = ("success", MagicMock(), {}) + + # Skip Phabricator/Lando publication of the (mocked) output + mock_workflow.update_build = False + + with mock_phabricator as api: + mock_workflow.phabricator = api + revision = PhabricatorRevision.from_phabricator_trigger( + build_target_phid="PHID-HMBT-test", + phabricator=api, + ) + mock_workflow.start_analysis(revision) + + if uses_git: + assert git_repo.called and git_worker.called + assert not hg_repo.called and not hg_worker.called + # Git path uses the git cache, not the mercurial one + assert git_repo.call_args.kwargs["cache_root"] == mock_config.git_cache + else: + assert hg_repo.called and hg_worker.called + assert not git_repo.called and not git_worker.called + + # Reset settings for following tests + mock_config.mercurial_cache = None + mock_config.git_cache = None + mock_config.ssh_key = None From cf3be33bee9142036be3c8365160fb9b253e2ead Mon Sep 17 00:00:00 2001 From: nhadji Date: Thu, 9 Jul 2026 11:31:45 +0200 Subject: [PATCH 4/8] Bug 2048352 - Publish Git worker failures to Phabricator and Lando The publication layer only knew the fail:mercurial mode, so a fail:git result from the GitWorker fell through to "Unsupported publication": the Phabricator build was never marked as failed and stayed in a running state forever, and Lando was never notified. Handle fail:git in publish_analysis_phabricator with a failure message mirroring the Mercurial one, and publish the existing patch-failure warning to Lando for both VCS modes. --- bot/code_review_bot/analysis.py | 26 +++++++++++++++-- bot/tests/test_phabricator_analysis.py | 40 +++++++++++++++++++++++++- 2 files changed, 62 insertions(+), 4 deletions(-) diff --git a/bot/code_review_bot/analysis.py b/bot/code_review_bot/analysis.py index e6b33427d..751fe5215 100644 --- a/bot/code_review_bot/analysis.py +++ b/bot/code_review_bot/analysis.py @@ -117,6 +117,25 @@ def publish_analysis_phabricator(payload, phabricator_api): build.target_phid, BuildState.Fail, unit=[failure] ) + elif mode == "fail:git": + extra_content = "" + if build.missing_base_revision: + extra_content = f" because the parent revision ({build.base_revision}) does not exist on the target repository. If possible, you should publish that revision" + + failure = UnitResult( + namespace="code-review", + name="git", + result=UnitResultState.Fail, + details="WARNING: The code review bot failed to apply your patch{}.\n\n```{}```".format( + extra_content, extras["message"] + ), + format="remarkup", + duration=extras.get("duration", 0), + ) + phabricator_api.update_build_target( + build.target_phid, BuildState.Fail, unit=[failure] + ) + elif mode == "test_result": result = UnitResult( namespace="code-review", @@ -192,10 +211,11 @@ def publish_analysis_lando(payload, lando_warnings): except Exception as ex: logger.error(str(ex), exc_info=True) - elif mode == "fail:mercurial": - # Send mercurial message to Lando + elif mode in ("fail:mercurial", "fail:git"): + # Send patch application failure message to Lando logger.info( - "Publishing code review hg failure.", + "Publishing code review VCS failure.", + mode=mode, revision=build.revision["id"], diff=build.diff_id, ) diff --git a/bot/tests/test_phabricator_analysis.py b/bot/tests/test_phabricator_analysis.py index 31d74b0c4..da98413af 100644 --- a/bot/tests/test_phabricator_analysis.py +++ b/bot/tests/test_phabricator_analysis.py @@ -8,11 +8,14 @@ from unittest.mock import MagicMock import pytest -from libmozdata.phabricator import ConduitError +from libmozdata.phabricator import BuildState, ConduitError from code_review_bot import mercurial from code_review_bot import workflow as workflow_module from code_review_bot.analysis import ( + LANDO_FAILURE_HG_MESSAGE, + PhabricatorRevisionBuild, + publish_analysis_lando, publish_analysis_phabricator, ) from code_review_bot.config import RepositoryConf @@ -274,6 +277,41 @@ def test_publish_analysis_phabricator_reraises_other_conduit_errors(): publish_analysis_phabricator(payload, phabricator_api) +@pytest.mark.parametrize("missing_base", [False, True]) +def test_publish_analysis_phabricator_git_failure(missing_base): + """A fail:git worker output marks the Phabricator build as failed.""" + build = mock.MagicMock() + build.target_phid = "PHID-HMBT-test" + build.missing_base_revision = missing_base + build.base_revision = "abcdef123456" + + phabricator_api = mock.MagicMock() + payload = ("fail:git", build, {"message": "git apply failed", "duration": 1}) + publish_analysis_phabricator(payload, phabricator_api) + + phabricator_api.update_build_target.assert_called_once() + args, kwargs = phabricator_api.update_build_target.call_args + assert args == ("PHID-HMBT-test", BuildState.Fail) + unit = kwargs["unit"][0] + assert unit["name"] == "git" + assert unit["result"] == "fail" + assert "failed to apply your patch" in unit["details"] + # The missing parent revision is only mentioned when it is the cause + assert ("abcdef123456" in unit["details"]) is missing_base + + +def test_publish_analysis_lando_git_failure(): + """A fail:git worker output publishes the patch failure warning to Lando.""" + build = PhabricatorRevisionBuild(mock.MagicMock(), mock.MagicMock()) + build.revision = {"id": 51} + build.diff_id = 42 + + lando_api = mock.MagicMock() + publish_analysis_lando(("fail:git", build, {}), lando_api) + + lando_api.add_warning.assert_called_once_with(LANDO_FAILURE_HG_MESSAGE, 51, 42) + + def test_repository_conf_repo_type(): """repo_type is optional, defaults to hg, and can be set to git (additive).""" conf = RepositoryConf( From 533a0258390850c92a5bfbadcb7743acfdc159a1 Mon Sep 17 00:00:00 2001 From: nhadji Date: Thu, 9 Jul 2026 11:31:45 +0200 Subject: [PATCH 5/8] Bug 2048352 - Report missing base revisions and harden clean() on the Git path GitRepository.get_base_identifier eagerly fell back to the default revision when the base was missing locally, so apply_build could never set missing_base_revision on the build and the "your patch has been rebased" warning was never published to Phabricator. Return the base revision as-is, mirroring the Mercurial implementation, and let apply_build record the fallback. clean() silently kept the previous build's commits when default_revision was left to its bare HEAD default without an origin remote to resolve against. Resolve the pristine base explicitly (remote-tracking branch first) and raise a clear error when none can be determined, and check it out detached instead of moving a local branch. --- bot/code_review_bot/git.py | 32 ++++++++--------- bot/tests/test_git.py | 74 +++++++++++++++++++++++++++++++++++--- 2 files changed, 86 insertions(+), 20 deletions(-) diff --git a/bot/code_review_bot/git.py b/bot/code_review_bot/git.py index 235c9e0e7..e8d9d3f8c 100644 --- a/bot/code_review_bot/git.py +++ b/bot/code_review_bot/git.py @@ -198,23 +198,14 @@ def has_revision(self, revision): def get_base_identifier(self, needed_stack: list[PhabricatorPatch]) -> str: """Return the base identifier to apply patches against. - Unlike Mercurial, the base revision is already a Git hash, so there is no - Lando ``git2hg`` conversion: when the base is not available locally we fall + Unlike Mercurial, the base revision is already a Git hash, so there is + no Lando ``git2hg`` conversion. A base revision missing locally is + handled by ``apply_build``, which records it on the build and falls back to the default revision. """ if self.use_latest_revision: return self.default_revision - - base_revision = needed_stack[0].base_revision - if self.has_revision(base_revision): - return base_revision - - logger.warning( - "Base revision not available locally, using the default revision", - revision=base_revision, - default=self.default_revision, - ) - return self.default_revision + return needed_stack[0].base_revision @staticmethod def get_author(commit): @@ -402,9 +393,18 @@ def clean(self): # Return to the pristine base, dropping any previously applied commits. # Prefer the remote-tracking base so we also pick up upstream updates. upstream = f"origin/{self.default_revision}" - target = upstream if self.has_revision(upstream) else self.default_revision - self.repo.git.checkout(self.default_revision, force=True) - self.repo.git.reset("--hard", target) + if self.has_revision(upstream): + target = upstream + elif self.default_revision != "HEAD": + target = self.default_revision + else: + # A bare HEAD cannot identify a pristine base once patches have + # been committed on top of it + raise Exception( + "Cannot determine the base to reset to: configure default_revision " + "or make sure the repository has an origin remote" + ) + self.repo.git.checkout(target, force=True, detach=True) class GitWorker: diff --git a/bot/tests/test_git.py b/bot/tests/test_git.py index 55f46c7f1..c5ecca521 100644 --- a/bot/tests/test_git.py +++ b/bot/tests/test_git.py @@ -5,6 +5,7 @@ import os.path from unittest.mock import MagicMock +import pytest from conftest import MockBuild from git.exc import GitCommandError @@ -57,16 +58,16 @@ def test_has_revision(mock_mc_git): def test_get_base_identifier_no_git2hg(mock_mc_git): - """The git base is used directly: no Lando git2hg lookup, fall back to default.""" + """The git base is used directly: no Lando git2hg lookup.""" head = mock_mc_git.repo.head.commit.hexsha - # Base present locally -> returned as-is present = MagicMock(base_revision=head) assert mock_mc_git.get_base_identifier([present]) == head - # Base absent -> fall back to the default revision (no git2hg call) + # An unknown base is returned as-is: apply_build detects it is missing, + # records it on the build and falls back to the default revision absent = MagicMock(base_revision="abcdef123456") - assert mock_mc_git.get_base_identifier([absent]) == mock_mc_git.default_revision + assert mock_mc_git.get_base_identifier([absent]) == "abcdef123456" def test_apply_patches(PhabricatorMock, mock_mc_git): @@ -82,6 +83,11 @@ def test_apply_patches(PhabricatorMock, mock_mc_git): assert os.path.exists(target) assert open(target).read() == "First Line\nSecond Line\n" + # The unknown base revision is recorded on the build with the fallback used + assert build.missing_base_revision is True + assert build.base_revision == build.stack[0].base_revision + assert build.actual_base_revision == mock_mc_git.default_revision + # Commits (newest first): the two patches on top of the base Readme commits = list(mock_mc_git.repo.iter_commits()) assert [c.message.strip() for c in commits] == [ @@ -183,6 +189,66 @@ def test_clean_drops_previous_build(PhabricatorMock, mock_mc_git): assert not os.path.exists(os.path.join(mock_mc_git.dir, "try_task_config.json")) +def test_clean_requires_pristine_base(tmpdir): + """Without a configured default_revision nor an origin remote, clean() + fails loudly instead of silently keeping the previous build's commits.""" + from conftest import build_git_repository + + repo = build_git_repository(tmpdir, "no-default") + config = { + "name": "no-default", + "url": "https://github.com/mozilla/test", + "try_url": str(tmpdir.mkdir("no-default-try.git").realpath()), + "ssh_key": "privateSSHkey", + } + git_repo = GitRepository(config, str(tmpdir.realpath())) + git_repo._repo = repo + + with pytest.raises(Exception, match="configure default_revision"): + git_repo.clean() + + +def test_clean_picks_up_remote_updates(tmpdir, mock_mc_git): + """clean() resets onto the remote-tracking base, so upstream commits + landed since the last build are picked up (mirrors hg pull).""" + from git import Actor, Repo + + # Clone the repo to act as its origin, and move it one commit ahead + origin_dir = str(tmpdir.mkdir("origin-repo").realpath()) + origin = mock_mc_git.repo.clone(origin_dir) + with origin.config_writer() as cw: + cw.set_value("user", "name", "test") + cw.set_value("user", "email", "test") + cw.set_value("commit", "gpgsign", "false") + with open(os.path.join(origin_dir, "update.txt"), "w") as f: + f.write("upstream update") + origin.index.add(["update.txt"]) + actor = Actor("test", "test") + upstream_tip = origin.index.commit("upstream update", author=actor, committer=actor) + + mock_mc_git.repo.create_remote("origin", origin_dir) + mock_mc_git.clean() + + assert mock_mc_git.repo.head.commit.hexsha == upstream_tip.hexsha + assert os.path.exists(os.path.join(mock_mc_git.dir, "update.txt")) + # The remote in the local clone is untouched by cleanup + assert Repo(origin_dir).head.commit.hexsha == upstream_tip.hexsha + + +def test_worker_failure_git(PhabricatorMock, mock_mc_git): + """A non-retryable Git error yields a fail:git result with the error log.""" + build = make_build(PhabricatorMock) + error = GitCommandError("git apply", 128, b"fatal: corrupt patch at line 3") + mock_mc_git.apply_build = MagicMock(side_effect=error) + + worker = GitWorker() + mode, out_build, details = worker.run(mock_mc_git, build) + + assert mode == "fail:git" + assert out_build is build + assert "corrupt patch" in details["message"] + + def test_worker_run_success(PhabricatorMock, mock_mc_git): """Full success path: apply, configure try, push, return treeherder link.""" build = make_build(PhabricatorMock) From 43f407202c7ac210ec64760df5b035da838646de Mon Sep 17 00:00:00 2001 From: nhadji Date: Thu, 9 Jul 2026 14:35:13 +0200 Subject: [PATCH 6/8] Bug 2048352 - Fetch the Git deploy key from a dedicated Taskcluster secret The deploy key used to push to Git try repositories lives in its own Taskcluster secret (field ssh_privkey), separate from the bot's runtime configuration secret whose tokens are shared with other environments. Add an optional --git-ssh-key-secret argument (GIT_SSH_KEY_SECRET in the environment) naming that secret. When set, the bot fetches it and Git repositories push with this key, falling back to the global ssh_key otherwise. The secret name stays configuration so the key's final location can be changed without a code change. --- bot/code_review_bot/cli.py | 12 ++++++++++++ bot/code_review_bot/config.py | 6 ++++++ bot/code_review_bot/workflow.py | 10 +++++----- bot/tests/test_phabricator_analysis.py | 16 +++++++++++++++- 4 files changed, 38 insertions(+), 6 deletions(-) diff --git a/bot/code_review_bot/cli.py b/bot/code_review_bot/cli.py index 3e12e4029..c2a596220 100644 --- a/bot/code_review_bot/cli.py +++ b/bot/code_review_bot/cli.py @@ -55,6 +55,11 @@ def parse_cli(): help="Taskcluster Secret path", default=os.environ.get("TASKCLUSTER_SECRET"), ) + parser.add_argument( + "--git-ssh-key-secret", + help="Taskcluster secret holding the SSH deploy key used to push to Git try repositories", + default=os.environ.get("GIT_SSH_KEY_SECRET"), + ) parser.add_argument( "--mercurial-repository", help="Optional path to a up-to-date mercurial repository matching the analyzed revision.\n" @@ -116,6 +121,12 @@ def main(): # Setup libmozdata configuration setup_libmozdata("code-review-bot") + # Load the dedicated Git deploy key from its own secret when configured + git_ssh_key = None + if args.git_ssh_key_secret: + secret = taskcluster.get_service("secrets").get(args.git_ssh_key_secret) + git_ssh_key = secret["secret"]["ssh_privkey"] + # Setup settings before stats settings.setup( taskcluster.secrets["APP_CHANNEL"], @@ -124,6 +135,7 @@ def main(): taskcluster.secrets["ssh_key"], args.mercurial_repository, args.github_repository, + git_ssh_key=git_ssh_key, ) # Setup statistics diff --git a/bot/code_review_bot/config.py b/bot/code_review_bot/config.py index 2a44a1c80..b27db40fe 100644 --- a/bot/code_review_bot/config.py +++ b/bot/code_review_bot/config.py @@ -66,6 +66,9 @@ def __init__(self): # SSH Key used to push on try self.ssh_key = None + # SSH deploy key used to push on Git try repositories + self.git_ssh_key = None + # List of users that should trigger a new analysis # Indexed by their Phabricator ID self.user_blacklist = {} @@ -83,6 +86,7 @@ def setup( ssh_key=None, mercurial_cache=None, git_cache=None, + git_ssh_key=None, ): # Detect source from env if "TRY_TASK_ID" in os.environ and "TRY_TASK_GROUP_ID" in os.environ: @@ -169,6 +173,8 @@ def build_conf(nb, repo): # Fallback to mercurial cache to ease migration on production systems self.git_cache = self.mercurial_cache + self.git_ssh_key = git_ssh_key + def load_user_blacklist(self, usernames, phabricator_api): """ Load all black listed users from Phabricator API diff --git a/bot/code_review_bot/workflow.py b/bot/code_review_bot/workflow.py index 92b0afa56..a4c770444 100644 --- a/bot/code_review_bot/workflow.py +++ b/bot/code_review_bot/workflow.py @@ -272,9 +272,9 @@ def start_analysis(self, revision): "One of Mercurial cache or github cache must be configured to start analysis" ) - # Cannot run without ssh key - if not settings.ssh_key: - raise Exception("SSH Key must be configured to start analysis") + # Cannot run without an ssh key + if not settings.ssh_key and not settings.git_ssh_key: + raise Exception("An SSH key must be configured to start analysis") # Set the Phabricator build as running self.update_status(revision, state=BuildState.Work) @@ -305,8 +305,8 @@ def start_analysis(self, revision): "try_name": base_conf.try_name, "url": base_conf.url, "try_url": base_conf.try_url, - # Setup ssh identity (a GitHub deploy key) - "ssh_key": settings.ssh_key, + # Deploy key from the dedicated secret, fallback to the global key + "ssh_key": settings.git_ssh_key or settings.ssh_key, }, cache_root=settings.git_cache, ) diff --git a/bot/tests/test_phabricator_analysis.py b/bot/tests/test_phabricator_analysis.py index da98413af..c1c567423 100644 --- a/bot/tests/test_phabricator_analysis.py +++ b/bot/tests/test_phabricator_analysis.py @@ -326,7 +326,14 @@ def test_repository_conf_repo_type(): assert conf._replace(repo_type="git").repo_type == "git" -@pytest.mark.parametrize("repo_type, uses_git", [("git", True), ("hg", False)]) +@pytest.mark.parametrize( + "repo_type, uses_git, git_ssh_key", + [ + ("git", True, "GitDeployKey"), + ("git", True, None), + ("hg", False, None), + ], +) def test_start_analysis_selects_backend( mock_phabricator, mock_workflow, @@ -335,11 +342,13 @@ def test_start_analysis_selects_backend( monkeypatch, repo_type, uses_git, + git_ssh_key, ): """start_analysis picks the Git or Mercurial backend from repo_type.""" mock_config.mercurial_cache = tmpdir mock_config.git_cache = tmpdir mock_config.ssh_key = "Dummy Private SSH Key" + mock_config.git_ssh_key = git_ssh_key # Force the configured repository's backend type mock_config.repositories = [ @@ -375,11 +384,16 @@ def test_start_analysis_selects_backend( assert not hg_repo.called and not hg_worker.called # Git path uses the git cache, not the mercurial one assert git_repo.call_args.kwargs["cache_root"] == mock_config.git_cache + # The dedicated deploy key wins, falling back to the global key + expected_key = git_ssh_key or "Dummy Private SSH Key" + assert git_repo.call_args.kwargs["config"]["ssh_key"] == expected_key else: assert hg_repo.called and hg_worker.called assert not git_repo.called and not git_worker.called + assert hg_repo.call_args.kwargs["config"]["ssh_key"] == "Dummy Private SSH Key" # Reset settings for following tests mock_config.mercurial_cache = None mock_config.git_cache = None mock_config.ssh_key = None + mock_config.git_ssh_key = None From 1fc32c7ba66b0a8ee86ff79cd6e1504479f01ffd Mon Sep 17 00:00:00 2001 From: nhadji Date: Thu, 9 Jul 2026 17:20:19 +0200 Subject: [PATCH 7/8] Bug 2048352 - Import workflow lazily in tests to keep date mocking effective test_phabricator_analysis.py imported code_review_bot.workflow at module level, which binds taskcluster.utils.stringDate inside the workflow module at collection time, before the autouse mock_taskcluster_date fixture can patch it. The frozen date then never applied to Workflow.index, breaking the date assertions of test_index.py when the whole suite runs. Import the module inside the test that needs it, like the other tests do. --- bot/tests/test_phabricator_analysis.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/bot/tests/test_phabricator_analysis.py b/bot/tests/test_phabricator_analysis.py index c1c567423..1b09ea1b4 100644 --- a/bot/tests/test_phabricator_analysis.py +++ b/bot/tests/test_phabricator_analysis.py @@ -11,7 +11,6 @@ from libmozdata.phabricator import BuildState, ConduitError from code_review_bot import mercurial -from code_review_bot import workflow as workflow_module from code_review_bot.analysis import ( LANDO_FAILURE_HG_MESSAGE, PhabricatorRevisionBuild, @@ -345,6 +344,12 @@ def test_start_analysis_selects_backend( git_ssh_key, ): """start_analysis picks the Git or Mercurial backend from repo_type.""" + # Import lazily: a module-level import binds taskcluster.utils.stringDate in + # code_review_bot.workflow at collection time, before the autouse + # mock_taskcluster_date fixture can patch it, breaking the date assertions + # of unrelated tests (e.g. test_index.py) + from code_review_bot import workflow as workflow_module + mock_config.mercurial_cache = tmpdir mock_config.git_cache = tmpdir mock_config.ssh_key = "Dummy Private SSH Key" From 38599f0d59f898b0ba31f0907025808ec7b98bff Mon Sep 17 00:00:00 2001 From: nhadji Date: Mon, 20 Jul 2026 16:16:16 +0200 Subject: [PATCH 8/8] Bug 2048352 - Authenticate Git pushes with a GitHub App installation token Replace the SSH deploy key authentication with short-lived GitHub App installation tokens, as suggested by the maintainers. The bot reads GITHUB_APP_ID and GITHUB_APP_PRIVKEY from the runtime configuration secret, generates a token restricted to the try repository with simple-github (cached for the run, installation tokens are valid one hour), and pushes over HTTPS with the token injected at push time. The token is never written to disk nor to the git configuration, and read operations keep using plain urls as the repositories are public. This removes the ssh key temporary file handling and the dedicated --git-ssh-key-secret argument. --- bot/code_review_bot/cli.py | 16 ++--- bot/code_review_bot/config.py | 11 ++-- bot/code_review_bot/git.py | 87 +++++++++++++++++--------- bot/code_review_bot/workflow.py | 13 ++-- bot/requirements.txt | 1 + bot/tests/conftest.py | 1 - bot/tests/test_git.py | 55 +++++++++++++++- bot/tests/test_phabricator_analysis.py | 23 +++---- 8 files changed, 142 insertions(+), 65 deletions(-) diff --git a/bot/code_review_bot/cli.py b/bot/code_review_bot/cli.py index c2a596220..f5edcf7df 100644 --- a/bot/code_review_bot/cli.py +++ b/bot/code_review_bot/cli.py @@ -55,11 +55,6 @@ def parse_cli(): help="Taskcluster Secret path", default=os.environ.get("TASKCLUSTER_SECRET"), ) - parser.add_argument( - "--git-ssh-key-secret", - help="Taskcluster secret holding the SSH deploy key used to push to Git try repositories", - default=os.environ.get("GIT_SSH_KEY_SECRET"), - ) parser.add_argument( "--mercurial-repository", help="Optional path to a up-to-date mercurial repository matching the analyzed revision.\n" @@ -103,6 +98,8 @@ def main(): "ALLOWED_PATHS": ["*"], "task_failures_ignored": [], "ssh_key": None, + "GITHUB_APP_ID": None, + "GITHUB_APP_PRIVKEY": None, "user_blacklist": [], }, local_secrets=yaml.safe_load(args.configuration) @@ -121,12 +118,6 @@ def main(): # Setup libmozdata configuration setup_libmozdata("code-review-bot") - # Load the dedicated Git deploy key from its own secret when configured - git_ssh_key = None - if args.git_ssh_key_secret: - secret = taskcluster.get_service("secrets").get(args.git_ssh_key_secret) - git_ssh_key = secret["secret"]["ssh_privkey"] - # Setup settings before stats settings.setup( taskcluster.secrets["APP_CHANNEL"], @@ -135,7 +126,8 @@ def main(): taskcluster.secrets["ssh_key"], args.mercurial_repository, args.github_repository, - git_ssh_key=git_ssh_key, + github_app_id=taskcluster.secrets["GITHUB_APP_ID"], + github_app_privkey=taskcluster.secrets["GITHUB_APP_PRIVKEY"], ) # Setup statistics diff --git a/bot/code_review_bot/config.py b/bot/code_review_bot/config.py index b27db40fe..610ec75f4 100644 --- a/bot/code_review_bot/config.py +++ b/bot/code_review_bot/config.py @@ -66,8 +66,9 @@ def __init__(self): # SSH Key used to push on try self.ssh_key = None - # SSH deploy key used to push on Git try repositories - self.git_ssh_key = None + # GitHub App credentials used to push on Git try repositories + self.github_app_id = None + self.github_app_privkey = None # List of users that should trigger a new analysis # Indexed by their Phabricator ID @@ -86,7 +87,8 @@ def setup( ssh_key=None, mercurial_cache=None, git_cache=None, - git_ssh_key=None, + github_app_id=None, + github_app_privkey=None, ): # Detect source from env if "TRY_TASK_ID" in os.environ and "TRY_TASK_GROUP_ID" in os.environ: @@ -173,7 +175,8 @@ def build_conf(nb, repo): # Fallback to mercurial cache to ease migration on production systems self.git_cache = self.mercurial_cache - self.git_ssh_key = git_ssh_key + self.github_app_id = github_app_id + self.github_app_privkey = github_app_privkey def load_user_blacklist(self, usernames, phabricator_api): """ diff --git a/bot/code_review_bot/git.py b/bot/code_review_bot/git.py index e8d9d3f8c..b343d6439 100644 --- a/bot/code_review_bot/git.py +++ b/bot/code_review_bot/git.py @@ -1,4 +1,4 @@ -import atexit +import asyncio import json import os import re @@ -11,6 +11,7 @@ from git import Repo from git.exc import GitCommandError from libmozdata.phabricator import PhabricatorPatch +from simple_github import AppAuth, AppInstallationAuth from code_review_bot.sources.phabricator import PhabricatorBuild @@ -124,7 +125,8 @@ class GitRepository: Notable differences from the Mercurial implementation: - the base revision is already a Git hash, so there is no Lando ``git2hg`` lookup; - - authentication uses an SSH deploy key passed through ``GIT_SSH_COMMAND``. + - pushes are authenticated over HTTPS with a short-lived GitHub App + installation token generated at push time. """ def __init__(self, config, cache_root): @@ -147,24 +149,14 @@ def __init__(self, config, cache_root): self._repo = None - # Write the SSH (deploy) key from the configuration to a temporary file - _, self.ssh_key_path = tempfile.mkstemp(suffix=".key") - with open(self.ssh_key_path, "w") as f: - f.write(config["ssh_key"]) - os.chmod(self.ssh_key_path, 0o600) - self.git_ssh_command = f"ssh -i {self.ssh_key_path} -o StrictHostKeyChecking=no" - - # Remove the key when finished - atexit.register(self.end_of_life) + # GitHub App credentials used to generate short-lived push tokens + self.github_app_id = config.get("github_app_id") + self.github_app_privkey = config.get("github_app_privkey") + self._github_token = None def __str__(self): return self.name - def end_of_life(self): - if os.path.exists(self.ssh_key_path): - os.unlink(self.ssh_key_path) - logger.info("Removed ssh key") - @property def repo(self): """Lazily open the local Git repository.""" @@ -173,16 +165,55 @@ def repo(self): self._repo = Repo(self.dir) return self._repo + def github_token(self): + """Short-lived GitHub App installation token for the try repository. + + Generated on first use and cached for the run (a bot run is well within + the one hour validity of installation tokens). + """ + if self._github_token is None: + assert ( + self.github_app_id and self.github_app_privkey + ), "Missing GitHub App credentials" + self._github_token = asyncio.run(self._generate_github_token()) + return self._github_token + + async def _generate_github_token(self): + parts = urlparse(self.try_url) + assert ( + parts.netloc == "github.com" + ), "GitHub App tokens only support github.com repositories" + path = parts.path.strip("/").removesuffix(".git") + owner, _, repo = path.partition("/") + auth = AppInstallationAuth( + AppAuth(self.github_app_id, self.github_app_privkey), + owner, + repositories=[repo], + ) + try: + return await auth.get_token() + finally: + await auth.close() + + def authenticated_url(self, url): + """Inject an installation token in an HTTPS GitHub url. + + Other urls (e.g. local paths in the test suite) are returned unchanged. + """ + parts = urlparse(url) + if parts.scheme not in ("http", "https"): + return url + return f"{parts.scheme}://git:{self.github_token()}@{parts.netloc}{parts.path}" + def clone(self): + # Read operations use the plain url: the repositories are public, only + # pushes need authentication logger.info("Checking out git repository", repo=self.url, dir=self.dir) if os.path.isdir(os.path.join(self.dir, ".git")): self._repo = Repo(self.dir) - with self.repo.git.custom_environment(GIT_SSH_COMMAND=self.git_ssh_command): - self.repo.remotes.origin.fetch() + self.repo.remotes.origin.fetch() else: - self._repo = Repo.clone_from( - self.url, self.dir, env={"GIT_SSH_COMMAND": self.git_ssh_command} - ) + self._repo = Repo.clone_from(self.url, self.dir) logger.info("Full checkout finished") def has_revision(self, revision): @@ -360,13 +391,14 @@ def add_try_commit(self, build): self.repo.git.commit("--no-verify", "-m", message) def push_to_try(self): - """Push the current HEAD to the remote try repository over the deploy key.""" + """Push the current HEAD to the remote try repository.""" head = self.repo.head.commit logger.info("Pushing patches to try", rev=head.hexsha, branch=self.head_branch) - with self.repo.git.custom_environment(GIT_SSH_COMMAND=self.git_ssh_command): - self.repo.git.push( - self.try_url, f"HEAD:refs/heads/{self.head_branch}", force=True - ) + self.repo.git.push( + self.authenticated_url(self.try_url), + f"HEAD:refs/heads/{self.head_branch}", + force=True, + ) return head def clean(self): @@ -387,8 +419,7 @@ def clean(self): # Refresh from the remote when one is configured (mirrors hg pull) if any(remote.name == "origin" for remote in self.repo.remotes): - with self.repo.git.custom_environment(GIT_SSH_COMMAND=self.git_ssh_command): - self.repo.remotes.origin.fetch() + self.repo.remotes.origin.fetch() # Return to the pristine base, dropping any previously applied commits. # Prefer the remote-tracking base so we also pick up upstream updates. diff --git a/bot/code_review_bot/workflow.py b/bot/code_review_bot/workflow.py index a4c770444..638ee603d 100644 --- a/bot/code_review_bot/workflow.py +++ b/bot/code_review_bot/workflow.py @@ -272,9 +272,11 @@ def start_analysis(self, revision): "One of Mercurial cache or github cache must be configured to start analysis" ) - # Cannot run without an ssh key - if not settings.ssh_key and not settings.git_ssh_key: - raise Exception("An SSH key must be configured to start analysis") + # Cannot run without a push credential + if not settings.ssh_key and not settings.github_app_privkey: + raise Exception( + "An SSH key or GitHub App must be configured to start analysis" + ) # Set the Phabricator build as running self.update_status(revision, state=BuildState.Work) @@ -305,8 +307,9 @@ def start_analysis(self, revision): "try_name": base_conf.try_name, "url": base_conf.url, "try_url": base_conf.try_url, - # Deploy key from the dedicated secret, fallback to the global key - "ssh_key": settings.git_ssh_key or settings.ssh_key, + # GitHub App credentials to generate short-lived push tokens + "github_app_id": settings.github_app_id, + "github_app_privkey": settings.github_app_privkey, }, cache_root=settings.git_cache, ) diff --git a/bot/requirements.txt b/bot/requirements.txt index ea46d614f..4866ce8df 100644 --- a/bot/requirements.txt +++ b/bot/requirements.txt @@ -8,5 +8,6 @@ pyyaml==6.0.3 rs_parsepatch==0.4.6 sentry-sdk==2.60.0 setuptools==80.9.0 +simple-github==3.1.0 structlog==25.5.0 taskcluster==100.1.1 diff --git a/bot/tests/conftest.py b/bot/tests/conftest.py index f1448becf..087c0b0a8 100644 --- a/bot/tests/conftest.py +++ b/bot/tests/conftest.py @@ -1238,7 +1238,6 @@ def mock_mc_git(tmpdir): "url": "https://github.com/mozilla/test", "try_url": try_dir, "try_name": "try", - "ssh_key": "privateSSHkey", "default_revision": repo.active_branch.name, "head_branch": "code-review", } diff --git a/bot/tests/test_git.py b/bot/tests/test_git.py index c5ecca521..8e1b0b066 100644 --- a/bot/tests/test_git.py +++ b/bot/tests/test_git.py @@ -199,7 +199,6 @@ def test_clean_requires_pristine_base(tmpdir): "name": "no-default", "url": "https://github.com/mozilla/test", "try_url": str(tmpdir.mkdir("no-default-try.git").realpath()), - "ssh_key": "privateSSHkey", } git_repo = GitRepository(config, str(tmpdir.realpath())) git_repo._repo = repo @@ -249,6 +248,60 @@ def test_worker_failure_git(PhabricatorMock, mock_mc_git): assert "corrupt patch" in details["message"] +def test_github_token(monkeypatch, tmpdir): + """An installation token is generated from the App credentials, restricted + to the try repository, and cached for the run.""" + from conftest import build_git_repository + + repo = build_git_repository(tmpdir, "app-repo") + config = { + "name": "app-repo", + "url": "https://github.com/mozilla-releng/staging-firefox", + "try_url": "https://github.com/mozilla-releng/staging-firefox.git", + "github_app_id": 12345, + "github_app_privkey": "AppPrivateKey", + } + git_repo = GitRepository(config, str(tmpdir.realpath())) + git_repo._repo = repo + + calls = [] + + class FakeInstallationAuth: + def __init__(self, app_auth, owner, repositories): + calls.append((app_auth, owner, repositories)) + + async def get_token(self): + return "generated-token" + + async def close(self): + pass + + monkeypatch.setattr( + "code_review_bot.git.AppAuth", lambda app_id, privkey: (app_id, privkey) + ) + monkeypatch.setattr("code_review_bot.git.AppInstallationAuth", FakeInstallationAuth) + + assert git_repo.github_token() == "generated-token" + assert calls == [((12345, "AppPrivateKey"), "mozilla-releng", ["staging-firefox"])] + + # Cached: no second generation + assert git_repo.github_token() == "generated-token" + assert len(calls) == 1 + + # The token is injected in https urls only + assert ( + git_repo.authenticated_url( + "https://github.com/mozilla-releng/staging-firefox.git" + ) + == "https://git:generated-token@github.com/mozilla-releng/staging-firefox.git" + ) + + +def test_authenticated_url_local_paths(mock_mc_git): + """Local paths (as used by the test remotes) are never authenticated.""" + assert mock_mc_git.authenticated_url(mock_mc_git.try_url) == mock_mc_git.try_url + + def test_worker_run_success(PhabricatorMock, mock_mc_git): """Full success path: apply, configure try, push, return treeherder link.""" build = make_build(PhabricatorMock) diff --git a/bot/tests/test_phabricator_analysis.py b/bot/tests/test_phabricator_analysis.py index 1b09ea1b4..9f4f54f59 100644 --- a/bot/tests/test_phabricator_analysis.py +++ b/bot/tests/test_phabricator_analysis.py @@ -325,14 +325,7 @@ def test_repository_conf_repo_type(): assert conf._replace(repo_type="git").repo_type == "git" -@pytest.mark.parametrize( - "repo_type, uses_git, git_ssh_key", - [ - ("git", True, "GitDeployKey"), - ("git", True, None), - ("hg", False, None), - ], -) +@pytest.mark.parametrize("repo_type, uses_git", [("git", True), ("hg", False)]) def test_start_analysis_selects_backend( mock_phabricator, mock_workflow, @@ -341,7 +334,6 @@ def test_start_analysis_selects_backend( monkeypatch, repo_type, uses_git, - git_ssh_key, ): """start_analysis picks the Git or Mercurial backend from repo_type.""" # Import lazily: a module-level import binds taskcluster.utils.stringDate in @@ -353,7 +345,8 @@ def test_start_analysis_selects_backend( mock_config.mercurial_cache = tmpdir mock_config.git_cache = tmpdir mock_config.ssh_key = "Dummy Private SSH Key" - mock_config.git_ssh_key = git_ssh_key + mock_config.github_app_id = 12345 + mock_config.github_app_privkey = "AppPrivateKey" # Force the configured repository's backend type mock_config.repositories = [ @@ -389,9 +382,10 @@ def test_start_analysis_selects_backend( assert not hg_repo.called and not hg_worker.called # Git path uses the git cache, not the mercurial one assert git_repo.call_args.kwargs["cache_root"] == mock_config.git_cache - # The dedicated deploy key wins, falling back to the global key - expected_key = git_ssh_key or "Dummy Private SSH Key" - assert git_repo.call_args.kwargs["config"]["ssh_key"] == expected_key + # The GitHub App credentials are passed through + conf = git_repo.call_args.kwargs["config"] + assert conf["github_app_id"] == 12345 + assert conf["github_app_privkey"] == "AppPrivateKey" else: assert hg_repo.called and hg_worker.called assert not git_repo.called and not git_worker.called @@ -401,4 +395,5 @@ def test_start_analysis_selects_backend( mock_config.mercurial_cache = None mock_config.git_cache = None mock_config.ssh_key = None - mock_config.git_ssh_key = None + mock_config.github_app_id = None + mock_config.github_app_privkey = None