From 3bb88969c99cd7bd493b3ed3bbc382c2bc500a98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Radek=20Ma=C5=88=C3=A1k?= Date: Thu, 25 Jun 2026 19:45:40 +0200 Subject: [PATCH 01/10] rebase: preserve merge-only deltas from downstream manual merges When a downstream two-parent merge commit contains tree changes that do not exist in any replayed non-merge commit (e.g. manual conflict resolutions), those changes were silently dropped by RebaseBot. This commit implements the "later-run path" fix: - Add `_find_merge_only_deltas(gitwd, source, dest)` which locates all eligible downstream merge commits (two-parent, non-synthetic) in the replay window and compares each commit's actual tree against the `git merge-tree --write-tree parent1 parent2` auto-merge baseline. Any difference is a merge-only delta. - Add `_apply_merge_only_delta(gitwd, merge_commit, diff_output)` which restores the affected paths to their exact state in the original merge commit tree and records a synthetic commit: UPSTREAM: : Recover merge-only delta from merge {sha[:12]} - Call both functions at the end of `_do_rebase`, after all regular cherry-picks, so the synthetic carry commits appear in topo/history order. No-op deltas (content already present after replay) are silently skipped. - Add regression test `test_later_run_merge_only_delta_preserved` that exercises the full dry-run rebase entrypoint: 1. First rebase runs; its PR is merged into dest. 2. A downstream feature branch is merged into dest with an extra file staged only at merge time (merge-only content). 3. Source advances; the second (later-run) rebase must produce a synthetic carry commit restoring that file. --- rebasebot/bot.py | 188 ++++++++++++++++++++++++++++++++++++++++++ tests/test_rebases.py | 117 ++++++++++++++++++++++++++ 2 files changed, 305 insertions(+) diff --git a/rebasebot/bot.py b/rebasebot/bot.py index fd11499..86bd49a 100755 --- a/rebasebot/bot.py +++ b/rebasebot/bot.py @@ -20,6 +20,7 @@ import logging import os import shutil +import subprocess import sys from collections import defaultdict @@ -440,6 +441,183 @@ def _safe_cherry_pick( ) +def _find_merge_only_deltas(gitwd: git.Repo, source: GitHubBranch, dest: GitHubBranch) -> list: + """ + Find downstream two-parent merge commits with merge-only deltas in the later-run replay window. + + A merge-only delta exists when the actual merge commit tree differs from the tree that + ``git merge-tree --write-tree parent1 parent2`` would produce automatically. + + Returns a list of (merge_commit, diff_output) tuples in topo/history order. + Only looks for eligible merges in the "later-run path" (when a prior rebasebot merge exists). + """ + merge_base = gitwd.git.merge_base(f"source/{source.branch}", f"dest/{dest.branch}") + ancestry_path_merges = gitwd.git.log( + _COMMIT_LOG_FORMAT, "--ancestry-path", "-r", "--merges", f"{merge_base}..dest/{dest.branch}" + ).splitlines() + + last_rebase_merge = _find_last_rebase_merge_commit(gitwd, ancestry_path_merges) + if last_rebase_merge is None: + logging.info("No prior rebase merge found; skipping merge-only delta detection (first-run path)") + return [] + + cutoffs = [f"^{parent.hexsha}" for parent in last_rebase_merge.parents] + + merge_lines = gitwd.git.log( + "--reverse", "--topo-order", _COMMIT_LOG_FORMAT, "--merges", + *cutoffs, f"dest/{dest.branch}", + ).splitlines() + + result = [] + for line in merge_lines: + if not line.strip(): + continue + sha, _, _ = line.split(" || ", 2) + merge = gitwd.commit(sha) + parents = list(merge.parents) + if len(parents) != _MERGE_COMMIT_PARENT_COUNT: + continue + + # Exclude synthetic rebase merge commits (identified by tree == upstream parent's tree). + upstream_parent = _find_source_parent_commit(parents, gitwd) + if upstream_parent is not None and merge.tree.hexsha == upstream_parent.tree.hexsha: + logging.info("Skipping synthetic rebase merge %s during merge-only delta scan", sha[:12]) + continue + + # Compute auto-merge tree baseline via git merge-tree --write-tree. + p1, p2 = parents[0].hexsha, parents[1].hexsha + try: + proc = subprocess.run( + ["git", "merge-tree", "--write-tree", p1, p2], + capture_output=True, + text=True, + cwd=gitwd.working_dir, + ) + # merge-tree exits 0 for clean merge, 1 for conflicts. + # Either way the tree SHA is on the first line of stdout. + auto_tree = proc.stdout.strip().split("\n")[0].strip() if proc.stdout.strip() else "" + if not auto_tree: + logging.warning("merge-tree produced no tree SHA for %s; skipping", sha[:12]) + continue + except OSError as ex: + logging.warning("Could not run git merge-tree for %s: %s", sha[:12], ex) + continue + + if auto_tree == merge.tree.hexsha: + logging.info("No merge-only delta in merge %s", sha[:12]) + continue + + # Diff auto-merge tree against actual merge tree to find affected paths. + try: + diff = gitwd.git.diff_tree("--no-commit-id", "-r", auto_tree, merge.tree.hexsha) + except git.GitCommandError as ex: + logging.warning("Could not diff merge-only delta for %s: %s", sha[:12], ex) + continue + + if diff.strip(): + logging.info("Found merge-only delta in downstream merge %s", sha[:12]) + result.append((merge, diff)) + + return result + + +def _apply_merge_only_delta(gitwd: git.Repo, merge_commit: Commit, diff_output: str) -> None: + """ + Apply a synthetic carry commit that recovers merge-only content from a downstream merge commit. + + Restores affected paths to the exact state in the original merge commit tree, then creates + a commit with message ``UPSTREAM: : Recover merge-only delta from merge {sha[:12]}``. + + Raises RepoException if the commit cannot be applied cleanly. + """ + sha = merge_commit.hexsha + short_sha = sha[:12] + + # Parse the diff-tree output to classify affected paths. + added_or_modified: list[str] = [] + deleted: list[str] = [] + for line in diff_output.splitlines(): + if not line.startswith(":"): + continue + tab_parts = line.split("\t", 1) + if len(tab_parts) < 2: + continue + meta_fields = tab_parts[0].lstrip(":").split() + if len(meta_fields) < 5: + continue + status = meta_fields[4][0] + path = tab_parts[1].split("\t")[0] # handle renames: take src path + if status == "D": + deleted.append(path) + elif status == "R": + # Rename: remove old path, add new path + new_path_parts = tab_parts[1].split("\t") + deleted.append(new_path_parts[0]) + if len(new_path_parts) > 1: + added_or_modified.append(new_path_parts[1]) + else: + added_or_modified.append(path) + + # Check whether applying the delta would change the current HEAD. + has_change = False + for path in added_or_modified: + try: + head_blob = gitwd.git.rev_parse(f"HEAD:{path}").strip() + merge_blob = gitwd.git.rev_parse(f"{sha}:{path}").strip() + if head_blob != merge_blob: + has_change = True + break + except git.GitCommandError: + has_change = True + break + if not has_change: + for path in deleted: + try: + gitwd.git.rev_parse(f"HEAD:{path}") + has_change = True # File still exists at HEAD; deleting it is a change. + break + except git.GitCommandError: + pass # Already absent — not a change. + + if not has_change: + logging.info("Merge-only delta from %s is a no-op after replay; skipping", short_sha) + return + + # Restore added/modified paths to their state in the merge commit's tree. + if added_or_modified: + try: + gitwd.git.checkout(sha, "--", *added_or_modified) + except git.GitCommandError as ex: + raise RepoException( + f"Failed to restore merge-only delta paths from merge {short_sha}: {ex}" + ) from ex + + # Remove paths that were deleted in the merge commit relative to the auto-merge baseline. + for path in deleted: + try: + gitwd.git.rm("--force", path) + except git.GitCommandError: + pass # Already absent; ignore. + + # Verify there are actually staged changes (guard against no-op after checkout). + try: + gitwd.git.diff("--cached", "--exit-code") + logging.info("No staged changes after applying merge-only delta from %s; skipping", short_sha) + return + except git.GitCommandError: + pass # Non-zero exit code means staged changes exist — proceed. + + commit_msg = f"UPSTREAM: : Recover merge-only delta from merge {short_sha}" + try: + gitwd.git.commit("-m", commit_msg) + except git.GitCommandError as ex: + raise RepoException( + f"Failed to create synthetic carry commit for merge-only delta from {short_sha}: {ex}" + ) from ex + + logging.info("Created synthetic carry commit for merge-only delta from merge %s", short_sha) + + def _do_rebase( *, gitwd: git.Repo, @@ -521,6 +699,16 @@ def _do_rebase( gitwd.git.commit("-m", newest_bot_commit_message, "--author", key) + # Recover merge-only deltas from eligible downstream merge commits (later-run path). + # This handles the case where a prior rebasebot rebase was merged into dest, then a + # downstream manual merge with custom conflict resolution (merge-only content) appeared. + # Each such delta is preserved as a synthetic UPSTREAM: commit. + for _merge_commit, _diff_output in _find_merge_only_deltas(gitwd, source, dest): + logging.info("Recovering merge-only delta from downstream merge %s", _merge_commit.hexsha[:12]) + _apply_merge_only_delta(gitwd, _merge_commit, _diff_output) + + + def _prepare_rebase_branch(gitwd: git.Repo, source: GitHubBranch, dest: GitHubBranch) -> None: logging.info("Preparing rebase branch") diff --git a/tests/test_rebases.py b/tests/test_rebases.py index 974792f..ec82af6 100644 --- a/tests/test_rebases.py +++ b/tests/test_rebases.py @@ -848,3 +848,120 @@ def test_always_run_hooks_preserves_carry_commits(self, init_test_repositories, # The hook should have succeeded (exit 0), proving DOWNSTREAM_OWNERS existed # which means hooks ran on dest branch, not source branch assert "hook-verified-carry.txt" in os.listdir(tmpdir) + + + def test_later_run_merge_only_delta_preserved(self, init_test_repositories, fake_github_provider, tmpdir): + """ + Later-run path: a downstream manual merge with merge-only content must be preserved. + + Scenario: + 1. Source advances; first dry-run rebase runs, producing a rebase branch. + 2. The rebase PR is merged into dest (--no-ff, simulated locally). + 3. A downstream feature branch is merged into dest with merge-only content: + an extra file (merge_resolution.txt) is staged before the merge commit, + so it exists in the merge commit tree but NOT in the auto-merge baseline. + 4. Source advances again; the second (later-run) dry-run rebase runs. + 5. Assert the later run produces a synthetic UPSTREAM: commit that + restores merge_resolution.txt from the downstream merge commit. + """ + source, rebase, dest = init_test_repositories + # Initial state: + # source: S0 (test.go) + # dest: S0 → D1 (another_file.go, "UPSTREAM: : our cool addition") + + # Step 1: advance source (trigger first rebase). + CommitBuilder(source).add_file("baz.txt", "upstream v2").commit("second upstream commit") + + # ── First dry-run rebase ──────────────────────────────────────────────── + first_run_dir = os.path.join(tmpdir, "first_run") + os.makedirs(first_run_dir) + + def _make_args(src, rbs, dst, wdir): + a = MagicMock() + a.source = src + a.source_repo = None + a.dest = dst + a.rebase = rbs + a.working_dir = wdir + a.git_username = "test_rebasebot" + a.git_email = "test@rebasebot.ocp" + a.tag_policy = "soft" + a.bot_emails = [] + a.exclude_commits = [] + a.update_go_modules = False + a.conflict_policy = "auto" + a.ignore_manual_label = False + a.dry_run = True + a.always_run_hooks = False + a.title_prefix = "" + a.pre_rebase_hook = None + a.post_rebase_hook = None + a.pre_carry_commit_hook = None + a.pre_push_rebase_branch_hook = None + a.pre_create_pr_hook = None + a.source_ref_hook = None + return a + + args1 = _make_args(source, rebase, dest, first_run_dir) + result1 = cli.rebasebot_run(args1, slack_webhook=None, github_app_wrapper=fake_github_provider) + assert result1, "First rebase run should succeed" + + # ── Simulate merging the rebase PR into dest (--no-ff) ───────────────── + dest_repo = Repo(dest.url) + dest_repo.git.checkout(dest.branch) + + remote_name = "first_run_remote" + if remote_name not in [r.name for r in dest_repo.remotes]: + dest_repo.create_remote(remote_name, first_run_dir) + dest_repo.remotes[remote_name].fetch("rebase") + dest_repo.git.merge(f"{remote_name}/rebase", "--no-ff", "-m", "Merge rebase PR into main") + + # ── Create a feature branch and merge it with merge-only content ──────── + dest_repo.git.checkout("-b", "feature_branch") + feature_file_path = os.path.join(dest.url, "feature.txt") + with open(feature_file_path, "x", encoding="utf8") as fh: + fh.write("feature content\n") + dest_repo.git.add("feature.txt") + dest_repo.git.commit("-m", "add feature file") + + dest_repo.git.checkout(dest.branch) + # --no-commit so we can inject a merge-only file before the merge commit. + dest_repo.git.merge("feature_branch", "--no-ff", "--no-commit") + + resolution_path = os.path.join(dest.url, "merge_resolution.txt") + with open(resolution_path, "w", encoding="utf8") as fh: + fh.write("manually resolved\n") + dest_repo.git.add("merge_resolution.txt") + + dest_repo.git.commit("-m", "Manual merge: feature into main with merge-only resolution") + manual_merge_sha = dest_repo.head.commit.hexsha + + # ── Advance source for the second (later) rebase ──────────────────────── + CommitBuilder(source).add_file("qux.txt", "upstream v3").commit("third upstream commit") + + # ── Second dry-run rebase (the "later run") ───────────────────────────── + second_run_dir = os.path.join(tmpdir, "second_run") + os.makedirs(second_run_dir) + args2 = _make_args(source, rebase, dest, second_run_dir) + result2 = cli.rebasebot_run(args2, slack_webhook=None, github_app_wrapper=fake_github_provider) + assert result2, "Second (later) rebase run should succeed" + + # ── Assertions ─────────────────────────────────────────────────────────── + second_working = Repo(second_run_dir) + commit_summaries = [c.summary for c in second_working.iter_commits("rebase")] + + expected_carry_msg = ( + f"UPSTREAM: : Recover merge-only delta from merge {manual_merge_sha[:12]}" + ) + assert expected_carry_msg in commit_summaries, ( + f"Expected synthetic carry commit not found in rebase branch.\n" + f"Expected: {expected_carry_msg!r}\n" + f"Got commits: {commit_summaries!r}" + ) + + # The synthetic carry commit's tree must include merge_resolution.txt. + carry_commit = next(c for c in second_working.iter_commits("rebase") if c.summary == expected_carry_msg) + blob_names = {b.name for b in carry_commit.tree.blobs} + assert "merge_resolution.txt" in blob_names, ( + f"merge_resolution.txt missing from synthetic carry commit tree; found: {blob_names!r}" + ) From 6d7d7d9aaea6f078173701d9b494ba04260bb830 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Radek=20Ma=C5=88=C3=A1k?= Date: Thu, 25 Jun 2026 20:06:54 +0200 Subject: [PATCH 02/10] rebase: preserve merge-only carry order Interleave recovered merge-only carries with downstream replay so they land in their historical position instead of being appended after later commits. --- rebasebot/bot.py | 74 ++++++++++++++++++----------- tests/test_rebases.py | 106 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 154 insertions(+), 26 deletions(-) diff --git a/rebasebot/bot.py b/rebasebot/bot.py index 86bd49a..e01b606 100755 --- a/rebasebot/bot.py +++ b/rebasebot/bot.py @@ -180,7 +180,9 @@ def _find_source_parent_commit(parents: list, gitwd: git.Repo) -> Commit: return None -def _identify_downstream_commits(gitwd: git.Repo, source: GitHubBranch, dest: GitHubBranch) -> str: +def _identify_downstream_commit_phases( + gitwd: git.Repo, source: GitHubBranch, dest: GitHubBranch +) -> tuple[list[str], list[str], list[str]]: # Merge base is the last shared commit of source branch and destination branch merge_base = gitwd.git.merge_base(f"source/{source.branch}", f"dest/{dest.branch}") logging.info(f"Merge base of source/{source.branch} and dest/{dest.branch}: %s", merge_base) @@ -223,10 +225,11 @@ def _identify_downstream_commits(gitwd: git.Repo, source: GitHubBranch, dest: Gi if not downstream_shas: logging.info("No downstream commits identified") - return "" + return [], [], cutoff_commits ordered_commits = [] seen = set() + phase1_lines = [] # Phase 1: Extract commits from the previous rebase PR first. # The rebase PR carries establish the baseline and must be cherry-picked @@ -276,7 +279,6 @@ def _identify_downstream_commits(gitwd: git.Repo, source: GitHubBranch, dest: Gi ).splitlines() # Keep only commits that are part of downstream set and not yet # emitted, so phase 1 establishes the carry baseline first. - phase1_lines = [] phase1_extras = [] for line in rebase_commits: sha = line.split(" || ", 1)[0].strip() @@ -312,7 +314,12 @@ def _identify_downstream_commits(gitwd: git.Repo, source: GitHubBranch, dest: Gi "\n".join(phase2_lines) if phase2_lines else "(none)", ) logging.info("Total downstream commits: %d", len(ordered_commits)) - downstream_commits = "\n".join(ordered_commits) + return phase1_lines, phase2_lines, cutoff_commits + + +def _identify_downstream_commits(gitwd: git.Repo, source: GitHubBranch, dest: GitHubBranch) -> str: + phase1_lines, phase2_lines, _ = _identify_downstream_commit_phases(gitwd, source, dest) + downstream_commits = "\n".join([*phase1_lines, *phase2_lines]) return downstream_commits @@ -551,10 +558,10 @@ def _apply_merge_only_delta(gitwd: git.Repo, merge_commit: Commit, diff_output: deleted.append(path) elif status == "R": # Rename: remove old path, add new path - new_path_parts = tab_parts[1].split("\t") - deleted.append(new_path_parts[0]) - if len(new_path_parts) > 1: - added_or_modified.append(new_path_parts[1]) + renamed_paths = tab_parts[1].split("\t") + deleted.append(renamed_paths[0]) + if len(renamed_paths) > 1: + added_or_modified.append(renamed_paths[1]) else: added_or_modified.append(path) @@ -636,29 +643,32 @@ def _do_rebase( if allow_bot_squash: logging.info("Bot squashing is enabled.") - downstream_commits = _identify_downstream_commits(gitwd, source, dest) + phase1_lines, phase2_lines, cutoff_commits = _identify_downstream_commit_phases(gitwd, source, dest) + phase1_shas = {line.split(" || ", 1)[0].strip() for line in phase1_lines if line.strip()} + phase2_shas = {line.split(" || ", 1)[0].strip() for line in phase2_lines if line.strip()} + merge_only_deltas = {merge.hexsha: (merge, diff) for merge, diff in _find_merge_only_deltas(gitwd, source, dest)} commits_to_squash = defaultdict(list) - for commit_line in downstream_commits.splitlines(): + def _replay_commit_line(commit_line: str) -> None: # Commit contains the message for logging purposes, # trim on the first space to get just the commit sha sha, commit_message, committer_email = commit_line.split(" || ", 2) if _in_excluded_commits(sha, exclude_commits): logging.info("Explicitly dropping commit from rebase: %s", sha) - continue + return if update_go_modules: # If we find a commit with such name, we know that it is a go mod update commit # and append such commit to a list of commits that we want to prune if commit_message == "UPSTREAM: : Updating and vendoring " + "go modules after an upstream rebase": logging.info("Dropping Go modules commit %s - %s", sha, commit_message) - continue + return if not _add_to_rebase(commit_message, source_repo, tag_policy, gitwd, source.branch): logging.info("Dropping commit: %s - %s", sha, commit_message) - continue + return if allow_bot_squash: # There is sometimes a prefix with number and a following + sign @@ -667,7 +677,7 @@ def _do_rebase( email = committer_email.split("+")[-1] if email in bot_emails: commits_to_squash[email].append({"sha": sha, "commit_message": commit_message}) - continue + return logging.info("Picking commit: %s - %s", sha, commit_message) @@ -679,6 +689,30 @@ def _do_rebase( commit_description=f"{sha} - {commit_message}", ) + for commit_line in phase1_lines: + _replay_commit_line(commit_line) + + phase2_history_lines = gitwd.git.log( + "--reverse", "--topo-order", _COMMIT_LOG_FORMAT, *cutoff_commits, f"dest/{dest.branch}" + ).splitlines() + for history_line in phase2_history_lines: + if not history_line.strip(): + continue + + sha, _, _ = history_line.split(" || ", 2) + if sha in phase1_shas: + continue + + merge_only_delta = merge_only_deltas.get(sha) + if merge_only_delta is not None: + merge_commit, diff_output = merge_only_delta + logging.info("Recovering merge-only delta from downstream merge %s", merge_commit.hexsha[:12]) + _apply_merge_only_delta(gitwd, merge_commit, diff_output) + continue + + if sha in phase2_shas: + _replay_commit_line(history_line) + # Here we cherry-pick the bot's commits and then squash them together # We also want the newest bot commit message to represent the squashed commits if allow_bot_squash: @@ -697,18 +731,6 @@ def _do_rebase( newest_bot_commit_message = value[-1]["commit_message"] gitwd.git.commit("-m", newest_bot_commit_message, "--author", key) - - - # Recover merge-only deltas from eligible downstream merge commits (later-run path). - # This handles the case where a prior rebasebot rebase was merged into dest, then a - # downstream manual merge with custom conflict resolution (merge-only content) appeared. - # Each such delta is preserved as a synthetic UPSTREAM: commit. - for _merge_commit, _diff_output in _find_merge_only_deltas(gitwd, source, dest): - logging.info("Recovering merge-only delta from downstream merge %s", _merge_commit.hexsha[:12]) - _apply_merge_only_delta(gitwd, _merge_commit, _diff_output) - - - def _prepare_rebase_branch(gitwd: git.Repo, source: GitHubBranch, dest: GitHubBranch) -> None: logging.info("Preparing rebase branch") diff --git a/tests/test_rebases.py b/tests/test_rebases.py index ec82af6..dce1237 100644 --- a/tests/test_rebases.py +++ b/tests/test_rebases.py @@ -965,3 +965,109 @@ def _make_args(src, rbs, dst, wdir): assert "merge_resolution.txt" in blob_names, ( f"merge_resolution.txt missing from synthetic carry commit tree; found: {blob_names!r}" ) + + def test_later_run_merge_only_delta_is_replayed_before_later_downstream_commits( + self, init_test_repositories, fake_github_provider, tmpdir + ): + """ + Later-run path: a recovered merge-only carry must land before later downstream commits. + + This isolates the ordering contract: if a downstream manual merge is followed by + another ordinary downstream commit, the synthetic carry recovered from that merge + must appear before the later commit on the rebased branch. + """ + source, rebase, dest = init_test_repositories + CommitBuilder(source).add_file("baz.txt", "upstream v2").commit("second upstream commit") + + first_run_dir = os.path.join(tmpdir, "first_run") + os.makedirs(first_run_dir) + + def _make_args(src, rbs, dst, wdir): + a = MagicMock() + a.source = src + a.source_repo = None + a.dest = dst + a.rebase = rbs + a.working_dir = wdir + a.git_username = "test_rebasebot" + a.git_email = "test@rebasebot.ocp" + a.tag_policy = "soft" + a.bot_emails = [] + a.exclude_commits = [] + a.update_go_modules = False + a.conflict_policy = "auto" + a.ignore_manual_label = False + a.dry_run = True + a.always_run_hooks = False + a.title_prefix = "" + a.pre_rebase_hook = None + a.post_rebase_hook = None + a.pre_carry_commit_hook = None + a.pre_push_rebase_branch_hook = None + a.pre_create_pr_hook = None + a.source_ref_hook = None + return a + + args1 = _make_args(source, rebase, dest, first_run_dir) + assert cli.rebasebot_run(args1, slack_webhook=None, github_app_wrapper=fake_github_provider) + + dest_repo = Repo(dest.url) + dest_repo.git.checkout(dest.branch) + + remote_name = "first_run_remote" + if remote_name not in [r.name for r in dest_repo.remotes]: + dest_repo.create_remote(remote_name, first_run_dir) + dest_repo.remotes[remote_name].fetch("rebase") + dest_repo.git.merge(f"{remote_name}/rebase", "--no-ff", "-m", "Merge rebase PR into main") + + dest_repo.git.checkout("-b", "feature_branch") + feature_file_path = os.path.join(dest.url, "feature.txt") + with open(feature_file_path, "x", encoding="utf8") as fh: + fh.write("feature content\n") + dest_repo.git.add("feature.txt") + dest_repo.git.commit("-m", "add feature file") + + dest_repo.git.checkout(dest.branch) + dest_repo.git.merge("feature_branch", "--no-ff", "--no-commit") + + resolution_path = os.path.join(dest.url, "merge_resolution.txt") + with open(resolution_path, "w", encoding="utf8") as fh: + fh.write("manually resolved\n") + dest_repo.git.add("merge_resolution.txt") + dest_repo.git.commit("-m", "Manual merge: feature into main with merge-only resolution") + manual_merge_sha = dest_repo.head.commit.hexsha + + follow_up_msg = "later downstream follow-up" + CommitBuilder(dest).add_file("post_merge_followup.txt", "follow-up content\n").commit(follow_up_msg) + + CommitBuilder(source).add_file("qux.txt", "upstream v3").commit("third upstream commit") + + second_run_dir = os.path.join(tmpdir, "second_run") + os.makedirs(second_run_dir) + args2 = _make_args(source, rebase, dest, second_run_dir) + assert cli.rebasebot_run(args2, slack_webhook=None, github_app_wrapper=fake_github_provider) + + second_working = Repo(second_run_dir) + chronological_summaries = [commit.summary for commit in reversed(list(second_working.iter_commits("rebase")))] + + expected_carry_msg = ( + f"UPSTREAM: : Recover merge-only delta from merge {manual_merge_sha[:12]}" + ) + assert expected_carry_msg in chronological_summaries, ( + f"Expected synthetic carry commit not found in rebase branch.\n" + f"Expected: {expected_carry_msg!r}\n" + f"Got commits: {chronological_summaries!r}" + ) + assert follow_up_msg in chronological_summaries, ( + f"Expected later downstream commit not found in rebase branch.\n" + f"Expected: {follow_up_msg!r}\n" + f"Got commits: {chronological_summaries!r}" + ) + replayed_follow_up_index = max( + index for index, summary in enumerate(chronological_summaries) if summary == follow_up_msg + ) + assert chronological_summaries.index(expected_carry_msg) < replayed_follow_up_index, ( + "Recovered merge-only carry was replayed after a later downstream commit.\n" + f"Expected {expected_carry_msg!r} before {follow_up_msg!r}.\n" + f"Got commits: {chronological_summaries!r}" + ) From 7a1ae4d637b34cb262f77921d25290cf04142db8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Radek=20Ma=C5=88=C3=A1k?= Date: Thu, 25 Jun 2026 20:16:43 +0200 Subject: [PATCH 03/10] rebase: extend merge-only delta recovery to first-run onboarding path When no prior RebaseBot synthetic merge marker exists on the dest branch (i.e., the first automated rebase), _find_merge_only_deltas now scans for eligible downstream merge commits using the merge_base as the cutoff instead of returning an empty list. Legacy downstream histories that previously synced upstream through real merge commits (with merge-only content such as manual conflict resolutions) will now have that content preserved during the first automated rebase, using the same synthetic UPSTREAM: semantics established for later-run histories. Existing ordinary merged-branch replay behavior is unchanged: only merges with a non-empty merge-only delta get a recovery carry. --- rebasebot/bot.py | 11 ++++-- tests/test_rebases.py | 89 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 4 deletions(-) diff --git a/rebasebot/bot.py b/rebasebot/bot.py index e01b606..746a55e 100755 --- a/rebasebot/bot.py +++ b/rebasebot/bot.py @@ -465,10 +465,13 @@ def _find_merge_only_deltas(gitwd: git.Repo, source: GitHubBranch, dest: GitHubB last_rebase_merge = _find_last_rebase_merge_commit(gitwd, ancestry_path_merges) if last_rebase_merge is None: - logging.info("No prior rebase merge found; skipping merge-only delta detection (first-run path)") - return [] - - cutoffs = [f"^{parent.hexsha}" for parent in last_rebase_merge.parents] + logging.info( + "No prior rebase merge found; scanning for merge-only deltas from merge_base " + "(first-run onboarding path)" + ) + cutoffs = [f"^{merge_base}"] + else: + cutoffs = [f"^{parent.hexsha}" for parent in last_rebase_merge.parents] merge_lines = gitwd.git.log( "--reverse", "--topo-order", _COMMIT_LOG_FORMAT, "--merges", diff --git a/tests/test_rebases.py b/tests/test_rebases.py index dce1237..c9c3616 100644 --- a/tests/test_rebases.py +++ b/tests/test_rebases.py @@ -966,6 +966,95 @@ def _make_args(src, rbs, dst, wdir): f"merge_resolution.txt missing from synthetic carry commit tree; found: {blob_names!r}" ) + def test_first_run_legacy_merge_only_delta_preserved(self, init_test_repositories, fake_github_provider, tmpdir): + """ + First-run (onboarding) path: a legacy downstream merge with merge-only content + must be recovered during the first automated rebase. + + Scenario: + - Source and dest share common history; no prior RebaseBot marker exists on dest. + - Dest has a legacy feature-branch merge committed with merge-only content + (first_run_resolution.txt injected before the merge commit). + - Source advances; the first automated dry-run rebase runs. + - Assert a synthetic UPSTREAM: commit restoring first_run_resolution.txt + is present in the rebase branch. + """ + source, rebase, dest = init_test_repositories + # Initial state (no prior rebasebot marker on dest): + # source: S0 (test.go) + # dest: S0 → D1 (another_file.go, "UPSTREAM: : our cool addition") + + # Advance source to trigger the first rebase. + CommitBuilder(source).add_file("baz.txt", "upstream v2").commit("second upstream commit") + + # Create a legacy feature branch on dest and add a feature file. + dest_repo = Repo(dest.url) + dest_repo.git.checkout("-b", "legacy_feature") + with open(os.path.join(dest.url, "legacy_feature.txt"), "x", encoding="utf8") as fh: + fh.write("legacy feature content\n") + dest_repo.git.add("legacy_feature.txt") + dest_repo.git.commit("-m", "add legacy feature file") + + # Merge the legacy feature branch into dest/main with --no-commit so we can + # inject a merge-only file (first_run_resolution.txt) before the merge commit. + dest_repo.git.checkout(dest.branch) + dest_repo.git.merge("legacy_feature", "--no-ff", "--no-commit") + + with open(os.path.join(dest.url, "first_run_resolution.txt"), "w", encoding="utf8") as fh: + fh.write("first-run manual resolution\n") + dest_repo.git.add("first_run_resolution.txt") + + dest_repo.git.commit("-m", "Legacy upstream merge with merge-only resolution") + legacy_merge_sha = dest_repo.head.commit.hexsha + + # Run the first dry-run rebase (no prior rebasebot marker on dest). + args = MagicMock() + args.source = source + args.source_repo = None + args.dest = dest + args.rebase = rebase + args.working_dir = tmpdir + args.git_username = "test_rebasebot" + args.git_email = "test@rebasebot.ocp" + args.tag_policy = "soft" + args.bot_emails = [] + args.exclude_commits = [] + args.update_go_modules = False + args.conflict_policy = "auto" + args.ignore_manual_label = False + args.dry_run = True + args.always_run_hooks = False + args.title_prefix = "" + args.pre_rebase_hook = None + args.post_rebase_hook = None + args.pre_carry_commit_hook = None + args.pre_push_rebase_branch_hook = None + args.pre_create_pr_hook = None + args.source_ref_hook = None + + result = cli.rebasebot_run(args, slack_webhook=None, github_app_wrapper=fake_github_provider) + assert result, "First-run rebase should succeed" + + # Verify the synthetic carry commit was created for the first-run merge-only delta. + working_repo = Repo(tmpdir) + commit_summaries = [c.summary for c in working_repo.iter_commits("rebase")] + + expected_carry_msg = ( + f"UPSTREAM: : Recover merge-only delta from merge {legacy_merge_sha[:12]}" + ) + assert expected_carry_msg in commit_summaries, ( + f"Expected synthetic carry commit not found in first-run rebase branch.\n" + f"Expected: {expected_carry_msg!r}\n" + f"Got commits: {commit_summaries!r}" + ) + + # Verify first_run_resolution.txt is present in the synthetic carry commit's tree. + carry_commit = next(c for c in working_repo.iter_commits("rebase") if c.summary == expected_carry_msg) + blob_names = {b.name for b in carry_commit.tree.blobs} + assert "first_run_resolution.txt" in blob_names, ( + f"first_run_resolution.txt missing from synthetic carry commit tree; found: {blob_names!r}" + ) + def test_later_run_merge_only_delta_is_replayed_before_later_downstream_commits( self, init_test_repositories, fake_github_provider, tmpdir ): From cc9bb1b9989b5cba28af5ef627dcb3a6dfa5f2cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Radek=20Ma=C5=88=C3=A1k?= Date: Thu, 25 Jun 2026 20:32:14 +0200 Subject: [PATCH 04/10] rebase: harden merge-only delta recovery Preserve recovered carries through strict-tag replays, honor source-merge exclusions, and detect mode-only tree deltas so merge recovery stays safe and lossless. --- rebasebot/bot.py | 40 +++--- tests/test_rebases.py | 282 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 301 insertions(+), 21 deletions(-) diff --git a/rebasebot/bot.py b/rebasebot/bot.py index 746a55e..a551862 100755 --- a/rebasebot/bot.py +++ b/rebasebot/bot.py @@ -55,6 +55,7 @@ class PullRequestUpdateException(Exception): _COMMIT_LOG_FORMAT = "--pretty=format:%H || %s || %aE" _MERGE_COMMIT_PARENT_COUNT = 2 _LOST_LINE_LOG_LIMIT = 10 +_RECOVERED_MERGE_ONLY_CARRY_PREFIX = "UPSTREAM: : Recover merge-only delta from merge " def _message_slack(webhook_url: str, msg: str) -> None: @@ -143,6 +144,17 @@ def _in_excluded_commits(sha: str, exclude_commits: list) -> bool: return False +def _is_recovered_merge_only_carry(commit_message: str) -> bool: + return commit_message.startswith(_RECOVERED_MERGE_ONLY_CARRY_PREFIX) + + +def _tree_entry_for_path(gitwd: git.Repo, ref: str, path: str) -> str: + try: + return gitwd.git.ls_tree(ref, "--", path).strip() + except git.GitCommandError: + return "" + + def _find_last_rebase_merge_commit(gitwd: git.Repo, ancestry_path_merges) -> Commit: logging.info("Searching for merge commit from previous rebasebot run to identify downstream commits") for merge_line in ancestry_path_merges: @@ -568,26 +580,9 @@ def _apply_merge_only_delta(gitwd: git.Repo, merge_commit: Commit, diff_output: else: added_or_modified.append(path) - # Check whether applying the delta would change the current HEAD. - has_change = False - for path in added_or_modified: - try: - head_blob = gitwd.git.rev_parse(f"HEAD:{path}").strip() - merge_blob = gitwd.git.rev_parse(f"{sha}:{path}").strip() - if head_blob != merge_blob: - has_change = True - break - except git.GitCommandError: - has_change = True - break - if not has_change: - for path in deleted: - try: - gitwd.git.rev_parse(f"HEAD:{path}") - has_change = True # File still exists at HEAD; deleting it is a change. - break - except git.GitCommandError: - pass # Already absent — not a change. + # Compare full tree entries so mode-only and other non-text changes are not mistaken for no-ops. + affected_paths = list(dict.fromkeys([*added_or_modified, *deleted])) + has_change = any(_tree_entry_for_path(gitwd, "HEAD", path) != _tree_entry_for_path(gitwd, sha, path) for path in affected_paths) if not has_change: logging.info("Merge-only delta from %s is a no-op after replay; skipping", short_sha) @@ -673,7 +668,7 @@ def _replay_commit_line(commit_line: str) -> None: logging.info("Dropping commit: %s - %s", sha, commit_message) return - if allow_bot_squash: + if allow_bot_squash and not _is_recovered_merge_only_carry(commit_message): # There is sometimes a prefix with number and a following + sign # We have to get rid of that part to make sure to get # only the email of the bot. @@ -709,6 +704,9 @@ def _replay_commit_line(commit_line: str) -> None: merge_only_delta = merge_only_deltas.get(sha) if merge_only_delta is not None: merge_commit, diff_output = merge_only_delta + if _in_excluded_commits(merge_commit.hexsha, exclude_commits): + logging.info("Explicitly skipping merge-only delta recovery from merge %s", merge_commit.hexsha) + continue logging.info("Recovering merge-only delta from downstream merge %s", merge_commit.hexsha[:12]) _apply_merge_only_delta(gitwd, merge_commit, diff_output) continue diff --git a/tests/test_rebases.py b/tests/test_rebases.py index c9c3616..7911112 100644 --- a/tests/test_rebases.py +++ b/tests/test_rebases.py @@ -4,11 +4,14 @@ from dataclasses import dataclass from unittest.mock import ANY, MagicMock, patch +import git import pytest from git import Repo from rebasebot import cli from rebasebot.bot import ( + RepoException, + _apply_merge_only_delta, _init_working_dir, _needs_rebase, _prepare_rebase_branch, @@ -31,6 +34,58 @@ def fetch_remotes(self): self.working_repo.git.fetch("--all") +def _make_rebase_args( + source, + rebase, + dest, + working_dir: str, + *, + tag_policy: str = "soft", + bot_emails: list[str] | None = None, + exclude_commits: list[str] | None = None, + always_run_hooks: bool = False, +): + args = MagicMock() + args.source = source + args.source_repo = None + args.dest = dest + args.rebase = rebase + args.working_dir = working_dir + args.git_username = "test_rebasebot" + args.git_email = "test@rebasebot.ocp" + args.tag_policy = tag_policy + args.bot_emails = bot_emails or [] + args.exclude_commits = exclude_commits or [] + args.update_go_modules = False + args.conflict_policy = "auto" + args.ignore_manual_label = False + args.dry_run = True + args.always_run_hooks = always_run_hooks + args.title_prefix = "" + args.pre_rebase_hook = None + args.post_rebase_hook = None + args.pre_carry_commit_hook = None + args.pre_push_rebase_branch_hook = None + args.pre_create_pr_hook = None + args.source_ref_hook = None + return args + + +def _merge_rebase_branch_into_dest( + dest: GitHubBranch, + rebase_worktree: str, + remote_name: str, + message: str = "Merge rebase PR into main", +) -> Repo: + dest_repo = Repo(dest.url) + dest_repo.git.checkout(dest.branch) + if remote_name not in [remote.name for remote in dest_repo.remotes]: + dest_repo.create_remote(remote_name, rebase_worktree) + dest_repo.remotes[remote_name].fetch("rebase") + dest_repo.git.merge(f"{remote_name}/rebase", "--no-ff", "-m", message) + return dest_repo + + class TestBotInternalHelpers: @pytest.fixture def working_repo_context(self, init_test_repositories, fake_github_provider, tmpdir) -> WorkingRepoContext: @@ -1160,3 +1215,230 @@ def _make_args(src, rbs, dst, wdir): f"Expected {expected_carry_msg!r} before {follow_up_msg!r}.\n" f"Got commits: {chronological_summaries!r}" ) + + def test_recovered_merge_only_carry_survives_strict_tag_policy_and_bot_squash( + self, init_test_repositories, fake_github_provider, tmpdir + ): + source, rebase, dest = init_test_repositories + CommitBuilder(source).add_file("baz.txt", "upstream v2").commit("second upstream commit") + + first_run_dir = os.path.join(tmpdir, "first_run") + os.makedirs(first_run_dir) + args1 = _make_rebase_args(source, rebase, dest, first_run_dir) + assert cli.rebasebot_run(args1, slack_webhook=None, github_app_wrapper=fake_github_provider) + + dest_repo = _merge_rebase_branch_into_dest(dest, first_run_dir, "first_run_remote") + + dest_repo.git.checkout("-b", "feature_branch") + with open(os.path.join(dest.url, "feature.txt"), "x", encoding="utf8") as fh: + fh.write("feature content\n") + dest_repo.git.add("feature.txt") + dest_repo.git.commit("-m", "add feature file") + + dest_repo.git.checkout(dest.branch) + dest_repo.git.merge("feature_branch", "--no-ff", "--no-commit") + with open(os.path.join(dest.url, "merge_resolution.txt"), "w", encoding="utf8") as fh: + fh.write("manually resolved\n") + dest_repo.git.add("merge_resolution.txt") + dest_repo.git.commit("-m", "Manual merge: feature into main with merge-only resolution") + manual_merge_sha = dest_repo.head.commit.hexsha + + CommitBuilder(source).add_file("qux.txt", "upstream v3").commit("third upstream commit") + + second_run_dir = os.path.join(tmpdir, "second_run") + os.makedirs(second_run_dir) + args2 = _make_rebase_args(source, rebase, dest, second_run_dir) + assert cli.rebasebot_run(args2, slack_webhook=None, github_app_wrapper=fake_github_provider) + + expected_recovery_msg = ( + f"UPSTREAM: : Recover merge-only delta from merge {manual_merge_sha[:12]}" + ) + assert expected_recovery_msg in [commit.summary for commit in Repo(second_run_dir).iter_commits("rebase")] + + _merge_rebase_branch_into_dest(dest, second_run_dir, "second_run_remote", "Merge second rebase PR into main") + + CommitBuilder(dest).add_file("generated-metadata.txt", "bot output\n").commit( + "UPSTREAM: : generated downstream metadata", + committer_email="test@rebasebot.ocp", + ) + + CommitBuilder(source).add_file("quux.txt", "upstream v4").commit("fourth upstream commit") + + third_run_dir = os.path.join(tmpdir, "third_run") + os.makedirs(third_run_dir) + args3 = _make_rebase_args( + source, + rebase, + dest, + third_run_dir, + tag_policy="strict", + bot_emails=["test@rebasebot.ocp"], + ) + assert cli.rebasebot_run(args3, slack_webhook=None, github_app_wrapper=fake_github_provider) + + replayed_summaries = Repo(third_run_dir).git.log("--first-parent", "--pretty=format:%s", "rebase").splitlines() + assert expected_recovery_msg in replayed_summaries, ( + "Recovered merge-only carry should survive a later strict-tag replay even when " + "bot squashing is enabled.\n" + f"Got first-parent commits: {replayed_summaries!r}" + ) + assert "UPSTREAM: : generated downstream metadata" in replayed_summaries, ( + "Control bot-authored carry commit should still be present in the strict-tag replay.\n" + f"Got first-parent commits: {replayed_summaries!r}" + ) + + def test_excluding_source_merge_sha_suppresses_merge_only_delta_recovery( + self, init_test_repositories, fake_github_provider, tmpdir + ): + source, rebase, dest = init_test_repositories + CommitBuilder(source).add_file("baz.txt", "upstream v2").commit("second upstream commit") + + first_run_dir = os.path.join(tmpdir, "first_run") + os.makedirs(first_run_dir) + args1 = _make_rebase_args(source, rebase, dest, first_run_dir) + assert cli.rebasebot_run(args1, slack_webhook=None, github_app_wrapper=fake_github_provider) + + dest_repo = _merge_rebase_branch_into_dest(dest, first_run_dir, "exclude_first_run_remote") + + dest_repo.git.checkout("-b", "feature_branch") + with open(os.path.join(dest.url, "feature.txt"), "x", encoding="utf8") as fh: + fh.write("feature content\n") + dest_repo.git.add("feature.txt") + dest_repo.git.commit("-m", "add feature file") + + dest_repo.git.checkout(dest.branch) + dest_repo.git.merge("feature_branch", "--no-ff", "--no-commit") + with open(os.path.join(dest.url, "merge_resolution.txt"), "w", encoding="utf8") as fh: + fh.write("manually resolved\n") + dest_repo.git.add("merge_resolution.txt") + dest_repo.git.commit("-m", "Manual merge: feature into main with merge-only resolution") + manual_merge_sha = dest_repo.head.commit.hexsha + + CommitBuilder(source).add_file("qux.txt", "upstream v3").commit("third upstream commit") + + second_run_dir = os.path.join(tmpdir, "second_run") + os.makedirs(second_run_dir) + args2 = _make_rebase_args( + source, + rebase, + dest, + second_run_dir, + exclude_commits=[manual_merge_sha], + ) + assert cli.rebasebot_run(args2, slack_webhook=None, github_app_wrapper=fake_github_provider) + + commit_summaries = [commit.summary for commit in Repo(second_run_dir).iter_commits("rebase")] + expected_recovery_msg = ( + f"UPSTREAM: : Recover merge-only delta from merge {manual_merge_sha[:12]}" + ) + assert expected_recovery_msg not in commit_summaries, ( + "Explicitly excluding the source merge SHA should suppress synthetic merge-only recovery.\n" + f"Got commits: {commit_summaries!r}" + ) + + def test_first_run_mode_only_merge_delta_is_preserved( + self, init_test_repositories, fake_github_provider, tmpdir + ): + source, rebase, dest = init_test_repositories + CommitBuilder(source).add_file("baz.txt", "upstream v2").commit("second upstream commit") + + dest_repo = Repo(dest.url) + dest_repo.git.checkout("-b", "mode_feature") + script_path = os.path.join(dest.url, "mode_only.sh") + with open(script_path, "x", encoding="utf8") as fh: + fh.write("#!/bin/sh\necho mode-only\n") + dest_repo.git.add("mode_only.sh") + dest_repo.git.commit("-m", "add mode-only script") + + dest_repo.git.checkout(dest.branch) + dest_repo.git.merge("mode_feature", "--no-ff", "--no-commit") + os.chmod(script_path, 0o755) + dest_repo.git.add("mode_only.sh") + dest_repo.git.commit("-m", "Legacy merge with mode-only resolution") + mode_merge_sha = dest_repo.head.commit.hexsha + + run_dir = os.path.join(tmpdir, "mode_run") + os.makedirs(run_dir) + args = _make_rebase_args(source, rebase, dest, run_dir) + assert cli.rebasebot_run(args, slack_webhook=None, github_app_wrapper=fake_github_provider) + + working_repo = Repo(run_dir) + expected_recovery_msg = ( + f"UPSTREAM: : Recover merge-only delta from merge {mode_merge_sha[:12]}" + ) + commit_summaries = [commit.summary for commit in working_repo.iter_commits("rebase")] + assert expected_recovery_msg in commit_summaries, ( + "Mode-only merge deltas should be recovered as synthetic carries.\n" + f"Got commits: {commit_summaries!r}" + ) + + carry_commit = next(commit for commit in working_repo.iter_commits("rebase") if commit.summary == expected_recovery_msg) + assert carry_commit.tree["mode_only.sh"].mode == 0o100755 + + def test_replay_time_no_op_merge_only_delta_is_skipped( + self, init_test_repositories, fake_github_provider, tmpdir + ): + source, rebase, dest = init_test_repositories + CommitBuilder(source).add_file("baz.txt", "upstream v2").commit("second upstream commit") + + first_run_dir = os.path.join(tmpdir, "first_run") + os.makedirs(first_run_dir) + args1 = _make_rebase_args(source, rebase, dest, first_run_dir) + assert cli.rebasebot_run(args1, slack_webhook=None, github_app_wrapper=fake_github_provider) + + dest_repo = _merge_rebase_branch_into_dest(dest, first_run_dir, "noop_first_run_remote") + + dest_repo.git.checkout("-b", "feature_branch") + with open(os.path.join(dest.url, "feature.txt"), "x", encoding="utf8") as fh: + fh.write("feature content\n") + dest_repo.git.add("feature.txt") + dest_repo.git.commit("-m", "add feature file") + + dest_repo.git.checkout(dest.branch) + dest_repo.git.merge("feature_branch", "--no-ff", "--no-commit") + with open(os.path.join(dest.url, "merge_resolution.txt"), "w", encoding="utf8") as fh: + fh.write("manually resolved\n") + dest_repo.git.add("merge_resolution.txt") + dest_repo.git.commit("-m", "Manual merge: feature into main with merge-only resolution") + manual_merge_sha = dest_repo.head.commit.hexsha + + CommitBuilder(source).add_file("merge_resolution.txt", "manually resolved\n").commit( + "upstream adopts merge resolution" + ) + + second_run_dir = os.path.join(tmpdir, "second_run") + os.makedirs(second_run_dir) + args2 = _make_rebase_args(source, rebase, dest, second_run_dir) + assert cli.rebasebot_run(args2, slack_webhook=None, github_app_wrapper=fake_github_provider) + + first_parent_summaries = Repo(second_run_dir).git.log("--first-parent", "--pretty=format:%s", "rebase").splitlines() + expected_recovery_msg = ( + f"UPSTREAM: : Recover merge-only delta from merge {manual_merge_sha[:12]}" + ) + assert expected_recovery_msg not in first_parent_summaries, ( + "Replay-time no-op merge-only deltas should be skipped instead of materializing empty carries.\n" + f"Got first-parent commits: {first_parent_summaries!r}" + ) + assert os.path.exists(os.path.join(second_run_dir, "merge_resolution.txt")) + + def test_recovery_requires_manual_intervention_when_synthetic_carry_cannot_apply_cleanly(self): + gitwd = MagicMock() + merge_commit = MagicMock() + merge_commit.hexsha = "1234567890abcdef1234567890abcdef12345678" + + gitwd.git.ls_tree.side_effect = [ + "100644 blob deadbeef\tmerge_resolution.txt", + "100644 blob feedface\tmerge_resolution.txt", + ] + gitwd.git.checkout.side_effect = git.GitCommandError( + "checkout", + 1, + stderr="would overwrite conflicting path", + ) + + with pytest.raises(RepoException, match="Failed to restore merge-only delta paths from merge 1234567890ab"): + _apply_merge_only_delta( + gitwd, + merge_commit, + ":000000 100644 0000000 1111111 A\tmerge_resolution.txt", + ) From 3286ded5157e6d4c9650160803c72e71af0159bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Radek=20Ma=C5=88=C3=A1k?= Date: Thu, 25 Jun 2026 20:50:24 +0200 Subject: [PATCH 05/10] tests: lock in operator-visibility log assertions for merge-only delta recovery Add four focused regression tests that assert INFO-level detection, recovery, no-op skip, and excluded-skip log messages are emitted during merge-only delta handling. Also add a WARNING log in _apply_merge_only_delta before the RepoException raise so the manual-intervention path is explicitly visible in operator logs. TDD: all four tests were written without caplog.set_level / without the new WARNING log (confirmed RED), then caplog.set_level(logging.INFO) was added to the integration tests and logging.warning() was added to _apply_merge_only_delta to make them GREEN. --- rebasebot/bot.py | 4 + tests/test_rebases.py | 196 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 200 insertions(+) diff --git a/rebasebot/bot.py b/rebasebot/bot.py index a551862..9e95998 100755 --- a/rebasebot/bot.py +++ b/rebasebot/bot.py @@ -593,6 +593,10 @@ def _apply_merge_only_delta(gitwd: git.Repo, merge_commit: Commit, diff_output: try: gitwd.git.checkout(sha, "--", *added_or_modified) except git.GitCommandError as ex: + logging.warning( + "Cannot apply merge-only delta from merge %s cleanly; manual intervention required", + short_sha, + ) raise RepoException( f"Failed to restore merge-only delta paths from merge {short_sha}: {ex}" ) from ex diff --git a/tests/test_rebases.py b/tests/test_rebases.py index 7911112..a7c09dc 100644 --- a/tests/test_rebases.py +++ b/tests/test_rebases.py @@ -1,5 +1,6 @@ from __future__ import annotations +import logging import os from dataclasses import dataclass from unittest.mock import ANY, MagicMock, patch @@ -1442,3 +1443,198 @@ def test_recovery_requires_manual_intervention_when_synthetic_carry_cannot_apply merge_commit, ":000000 100644 0000000 1111111 A\tmerge_resolution.txt", ) + + def test_detection_and_recovery_logs_emitted( + self, init_test_repositories, fake_github_provider, tmpdir, caplog + ): + """ + Operator visibility: INFO logs for detection and recovery of a merge-only delta + must be emitted during the later-run rebase. + """ + caplog.set_level(logging.INFO) + source, rebase, dest = init_test_repositories + CommitBuilder(source).add_file("baz.txt", "upstream v2").commit("second upstream commit") + + first_run_dir = os.path.join(tmpdir, "first_run") + os.makedirs(first_run_dir) + assert cli.rebasebot_run( + _make_rebase_args(source, rebase, dest, first_run_dir), + slack_webhook=None, + github_app_wrapper=fake_github_provider, + ) + dest_repo = _merge_rebase_branch_into_dest(dest, first_run_dir, "detect_log_remote") + + dest_repo.git.checkout("-b", "feature_branch") + with open(os.path.join(dest.url, "feature.txt"), "x", encoding="utf8") as fh: + fh.write("feature content\n") + dest_repo.git.add("feature.txt") + dest_repo.git.commit("-m", "add feature file") + + dest_repo.git.checkout(dest.branch) + dest_repo.git.merge("feature_branch", "--no-ff", "--no-commit") + with open(os.path.join(dest.url, "detect_resolution.txt"), "w", encoding="utf8") as fh: + fh.write("manually resolved\n") + dest_repo.git.add("detect_resolution.txt") + dest_repo.git.commit("-m", "Manual merge: feature with detection log resolution") + manual_merge_sha = dest_repo.head.commit.hexsha + + CommitBuilder(source).add_file("qux.txt", "upstream v3").commit("third upstream commit") + + second_run_dir = os.path.join(tmpdir, "second_run") + os.makedirs(second_run_dir) + assert cli.rebasebot_run( + _make_rebase_args(source, rebase, dest, second_run_dir), + slack_webhook=None, + github_app_wrapper=fake_github_provider, + ) + + assert f"Found merge-only delta in downstream merge {manual_merge_sha[:12]}" in caplog.text, ( + "Expected detection INFO log to be captured. " + "Ensure caplog.set_level(logging.INFO) is set." + ) + assert ( + f"Created synthetic carry commit for merge-only delta from merge {manual_merge_sha[:12]}" + in caplog.text + ), "Expected recovery INFO log to be captured." + + def test_no_op_skip_log_emitted( + self, init_test_repositories, fake_github_provider, tmpdir, caplog + ): + """ + Operator visibility: INFO log for skipping a merge-only delta that becomes a no-op + after replay (because source has since adopted the same content). + """ + caplog.set_level(logging.INFO) + source, rebase, dest = init_test_repositories + CommitBuilder(source).add_file("baz.txt", "upstream v2").commit("second upstream commit") + + first_run_dir = os.path.join(tmpdir, "first_run") + os.makedirs(first_run_dir) + assert cli.rebasebot_run( + _make_rebase_args(source, rebase, dest, first_run_dir), + slack_webhook=None, + github_app_wrapper=fake_github_provider, + ) + dest_repo = _merge_rebase_branch_into_dest(dest, first_run_dir, "noop_log_remote") + + dest_repo.git.checkout("-b", "feature_branch") + with open(os.path.join(dest.url, "feature.txt"), "x", encoding="utf8") as fh: + fh.write("feature content\n") + dest_repo.git.add("feature.txt") + dest_repo.git.commit("-m", "add feature file") + + dest_repo.git.checkout(dest.branch) + dest_repo.git.merge("feature_branch", "--no-ff", "--no-commit") + with open(os.path.join(dest.url, "noop_resolution.txt"), "w", encoding="utf8") as fh: + fh.write("noop resolved\n") + dest_repo.git.add("noop_resolution.txt") + dest_repo.git.commit("-m", "Manual merge: feature with noop resolution") + manual_merge_sha = dest_repo.head.commit.hexsha + + # Source adopts the same content, making the carry a no-op at replay time. + CommitBuilder(source).add_file("noop_resolution.txt", "noop resolved\n").commit( + "upstream adopts noop resolution" + ) + + second_run_dir = os.path.join(tmpdir, "second_run") + os.makedirs(second_run_dir) + assert cli.rebasebot_run( + _make_rebase_args(source, rebase, dest, second_run_dir), + slack_webhook=None, + github_app_wrapper=fake_github_provider, + ) + + assert ( + f"Merge-only delta from {manual_merge_sha[:12]} is a no-op after replay; skipping" + in caplog.text + ), ( + "Expected no-op skip INFO log to be captured. " + "Ensure caplog.set_level(logging.INFO) is set." + ) + + def test_excluded_skip_log_emitted( + self, init_test_repositories, fake_github_provider, tmpdir, caplog + ): + """ + Operator visibility: INFO log when a merge SHA is explicitly excluded via + --exclude-commits, suppressing merge-only delta recovery. + """ + caplog.set_level(logging.INFO) + source, rebase, dest = init_test_repositories + CommitBuilder(source).add_file("baz.txt", "upstream v2").commit("second upstream commit") + + first_run_dir = os.path.join(tmpdir, "first_run") + os.makedirs(first_run_dir) + assert cli.rebasebot_run( + _make_rebase_args(source, rebase, dest, first_run_dir), + slack_webhook=None, + github_app_wrapper=fake_github_provider, + ) + dest_repo = _merge_rebase_branch_into_dest(dest, first_run_dir, "exclude_log_remote") + + dest_repo.git.checkout("-b", "feature_branch") + with open(os.path.join(dest.url, "feature.txt"), "x", encoding="utf8") as fh: + fh.write("feature content\n") + dest_repo.git.add("feature.txt") + dest_repo.git.commit("-m", "add feature file") + + dest_repo.git.checkout(dest.branch) + dest_repo.git.merge("feature_branch", "--no-ff", "--no-commit") + with open(os.path.join(dest.url, "exclude_resolution.txt"), "w", encoding="utf8") as fh: + fh.write("excluded resolution\n") + dest_repo.git.add("exclude_resolution.txt") + dest_repo.git.commit("-m", "Manual merge: feature with excluded resolution") + manual_merge_sha = dest_repo.head.commit.hexsha + + CommitBuilder(source).add_file("qux.txt", "upstream v3").commit("third upstream commit") + + second_run_dir = os.path.join(tmpdir, "second_run") + os.makedirs(second_run_dir) + assert cli.rebasebot_run( + _make_rebase_args( + source, rebase, dest, second_run_dir, exclude_commits=[manual_merge_sha] + ), + slack_webhook=None, + github_app_wrapper=fake_github_provider, + ) + + assert "Explicitly skipping merge-only delta recovery from merge" in caplog.text, ( + "Expected excluded-skip INFO log to be captured. " + "Ensure caplog.set_level(logging.INFO) is set." + ) + + def test_manual_intervention_warning_logged_on_apply_failure(self, caplog): + """ + Operator visibility: a WARNING log must be emitted specifically for the + merge-only delta recovery failure case before the RepoException is raised. + + This is a unit-level test using mocks. The WARNING log helps operators identify + the precise merge that required manual intervention without needing to parse + the generic outer error. + """ + gitwd = MagicMock() + merge_commit = MagicMock() + merge_commit.hexsha = "abcdef012345abcdef012345abcdef0123456789" + + gitwd.git.ls_tree.side_effect = [ + "100644 blob deadbeef\tconflict_file.txt", + "100644 blob feedface\tconflict_file.txt", + ] + gitwd.git.checkout.side_effect = git.GitCommandError( + "checkout", + 1, + stderr="would overwrite conflicting path", + ) + + with pytest.raises(RepoException): + _apply_merge_only_delta( + gitwd, + merge_commit, + ":000000 100644 0000000 1111111 A\tconflict_file.txt", + ) + + assert "abcdef012345" in caplog.text, ( + "Expected a WARNING log referencing the merge SHA when merge-only delta " + "recovery cannot be applied cleanly. " + "Add logging.warning(...) in _apply_merge_only_delta before the RepoException raise." + ) From 1b4fd8de24301cda49dc782a4945439e43394917 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Radek=20Ma=C5=88=C3=A1k?= Date: Thu, 25 Jun 2026 21:08:14 +0200 Subject: [PATCH 06/10] rebase: tighten merge-only delta detection Treat only RebaseBot's own synthetic upstream merge as excludable and stop the rebase when merge-delta baselines cannot be computed, so real upstream-sync resolutions are preserved instead of being skipped silently. --- rebasebot/bot.py | 120 +++++++++++++++++++++++++++++----------- tests/test_rebases.py | 124 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 213 insertions(+), 31 deletions(-) diff --git a/rebasebot/bot.py b/rebasebot/bot.py index 9e95998..9185ecc 100755 --- a/rebasebot/bot.py +++ b/rebasebot/bot.py @@ -58,6 +58,10 @@ class PullRequestUpdateException(Exception): _RECOVERED_MERGE_ONLY_CARRY_PREFIX = "UPSTREAM: : Recover merge-only delta from merge " +def _synthetic_rebase_merge_message(source_branch: str, dest_branch: str) -> str: + return f"merge upstream/{source_branch} into {dest_branch}" + + def _message_slack(webhook_url: str, msg: str) -> None: """Send a message to Slack via a webhook if one is configured.""" if webhook_url is None: @@ -149,35 +153,60 @@ def _is_recovered_merge_only_carry(commit_message: str) -> bool: def _tree_entry_for_path(gitwd: git.Repo, ref: str, path: str) -> str: - try: - return gitwd.git.ls_tree(ref, "--", path).strip() - except git.GitCommandError: - return "" + return gitwd.git.ls_tree(ref, "--", path).strip() + + +def _is_rebasebot_synthetic_merge( + merge: Commit, + gitwd: git.Repo, + source: GitHubBranch, + dest: GitHubBranch, + *, + first_parent_merge_shas: set[str], +) -> bool: + parents = list(merge.parents) + if len(parents) != _MERGE_COMMIT_PARENT_COUNT: + return False + + # RebaseBot's synthetic upstream merge lives on the rebase branch and is later + # introduced into dest via the PR merge, so it is not on dest's first-parent path. + if merge.hexsha in first_parent_merge_shas: + return False + if merge.summary != _synthetic_rebase_merge_message(source.branch, dest.branch): + return False + + upstream_parent = _find_source_parent_commit(parents, gitwd) + if upstream_parent is None: + return False -def _find_last_rebase_merge_commit(gitwd: git.Repo, ancestry_path_merges) -> Commit: + return merge.tree.hexsha == upstream_parent.tree.hexsha + + +def _find_last_rebase_merge_commit( + gitwd: git.Repo, + source: GitHubBranch, + dest: GitHubBranch, + ancestry_path_merges, + *, + first_parent_merge_shas: set[str], +) -> Commit: logging.info("Searching for merge commit from previous rebasebot run to identify downstream commits") for merge_line in ancestry_path_merges: sha, _, _ = merge_line.split(" || ", 2) merge = gitwd.commit(sha) - - # Last rebase merge commit has two parents. - parents = list(merge.parents) - if len(parents) != _MERGE_COMMIT_PARENT_COUNT: - continue - - # Identify the upstream parent: Merge parent that is reachable from any upstream branch. - upstream_parent = _find_source_parent_commit(parents, gitwd) - if upstream_parent is None: - continue - - # The synthetic rebase merge uses the upstream tree as the merge tree. - # Therefore, the merge commit's tree must equal the upstream parent's tree. - if merge.tree.hexsha != upstream_parent.tree.hexsha: + if not _is_rebasebot_synthetic_merge( + merge, + gitwd, + source, + dest, + first_parent_merge_shas=first_parent_merge_shas, + ): continue logging.info("Found merge commit from previous rebase: %s", sha) + upstream_parent = _find_source_parent_commit(list(merge.parents), gitwd) logging.info("Its parent %s is on an upstream branch", upstream_parent.hexsha) return merge return None @@ -203,11 +232,20 @@ def _identify_downstream_commit_phases( ancestry_path_merges = gitwd.git.log( _COMMIT_LOG_FORMAT, "--ancestry-path", "-r", "--merges", f"{merge_base}..dest/{dest.branch}" ).splitlines() + first_parent_merge_shas = set( + gitwd.git.rev_list("--first-parent", "--merges", f"{merge_base}..dest/{dest.branch}").splitlines() + ) val = "\n".join(ancestry_path_merges) logging.info(f"""Merges on ancestry-path from merge_base=({merge_base}) to dest/{dest.branch} branch:\n{val}""") - last_rebase_merge_commit = _find_last_rebase_merge_commit(gitwd, ancestry_path_merges) + last_rebase_merge_commit = _find_last_rebase_merge_commit( + gitwd, + source, + dest, + ancestry_path_merges, + first_parent_merge_shas=first_parent_merge_shas, + ) cutoff_commits = [] if last_rebase_merge_commit is None: @@ -475,7 +513,16 @@ def _find_merge_only_deltas(gitwd: git.Repo, source: GitHubBranch, dest: GitHubB _COMMIT_LOG_FORMAT, "--ancestry-path", "-r", "--merges", f"{merge_base}..dest/{dest.branch}" ).splitlines() - last_rebase_merge = _find_last_rebase_merge_commit(gitwd, ancestry_path_merges) + first_parent_merge_shas = set( + gitwd.git.rev_list("--first-parent", "--merges", f"{merge_base}..dest/{dest.branch}").splitlines() + ) + last_rebase_merge = _find_last_rebase_merge_commit( + gitwd, + source, + dest, + ancestry_path_merges, + first_parent_merge_shas=first_parent_merge_shas, + ) if last_rebase_merge is None: logging.info( "No prior rebase merge found; scanning for merge-only deltas from merge_base " @@ -489,6 +536,9 @@ def _find_merge_only_deltas(gitwd: git.Repo, source: GitHubBranch, dest: GitHubB "--reverse", "--topo-order", _COMMIT_LOG_FORMAT, "--merges", *cutoffs, f"dest/{dest.branch}", ).splitlines() + eligible_first_parent_merge_shas = set( + gitwd.git.rev_list("--first-parent", "--merges", *cutoffs, f"dest/{dest.branch}").splitlines() + ) result = [] for line in merge_lines: @@ -500,9 +550,13 @@ def _find_merge_only_deltas(gitwd: git.Repo, source: GitHubBranch, dest: GitHubB if len(parents) != _MERGE_COMMIT_PARENT_COUNT: continue - # Exclude synthetic rebase merge commits (identified by tree == upstream parent's tree). - upstream_parent = _find_source_parent_commit(parents, gitwd) - if upstream_parent is not None and merge.tree.hexsha == upstream_parent.tree.hexsha: + if _is_rebasebot_synthetic_merge( + merge, + gitwd, + source, + dest, + first_parent_merge_shas=eligible_first_parent_merge_shas, + ): logging.info("Skipping synthetic rebase merge %s during merge-only delta scan", sha[:12]) continue @@ -519,11 +573,14 @@ def _find_merge_only_deltas(gitwd: git.Repo, source: GitHubBranch, dest: GitHubB # Either way the tree SHA is on the first line of stdout. auto_tree = proc.stdout.strip().split("\n")[0].strip() if proc.stdout.strip() else "" if not auto_tree: - logging.warning("merge-tree produced no tree SHA for %s; skipping", sha[:12]) - continue + raise RepoException( + f"Failed to determine merge-only delta baseline for merge {sha[:12]}: " + f"git merge-tree produced no tree SHA (exit {proc.returncode})" + ) except OSError as ex: - logging.warning("Could not run git merge-tree for %s: %s", sha[:12], ex) - continue + raise RepoException( + f"Failed to determine merge-only delta baseline for merge {sha[:12]}: {ex}" + ) from ex if auto_tree == merge.tree.hexsha: logging.info("No merge-only delta in merge %s", sha[:12]) @@ -533,8 +590,9 @@ def _find_merge_only_deltas(gitwd: git.Repo, source: GitHubBranch, dest: GitHubB try: diff = gitwd.git.diff_tree("--no-commit-id", "-r", auto_tree, merge.tree.hexsha) except git.GitCommandError as ex: - logging.warning("Could not diff merge-only delta for %s: %s", sha[:12], ex) - continue + raise RepoException( + f"Failed to diff merge-only delta for merge {sha[:12]}: {ex}" + ) from ex if diff.strip(): logging.info("Found merge-only delta in downstream merge %s", sha[:12]) @@ -760,7 +818,7 @@ def _prepare_rebase_branch(gitwd: git.Repo, source: GitHubBranch, dest: GitHubBr "-p", MERGE_TMP_BRANCH, "-m", - f"merge upstream/{source.branch} into {dest.branch}", + _synthetic_rebase_merge_message(source.branch, dest.branch), ) logging.info(f"Merging upstream/{source.branch} into {dest.branch}") diff --git a/tests/test_rebases.py b/tests/test_rebases.py index a7c09dc..1b42486 100644 --- a/tests/test_rebases.py +++ b/tests/test_rebases.py @@ -16,6 +16,7 @@ _init_working_dir, _needs_rebase, _prepare_rebase_branch, + _tree_entry_for_path, ) from rebasebot.github import GitHubBranch, parse_github_branch @@ -143,6 +144,13 @@ def test_prepare_rebase_branch(self, working_repo_context): "Upstream commit", ] + def test_tree_entry_for_path_propagates_real_lookup_failures(self): + gitwd = MagicMock() + gitwd.git.ls_tree.side_effect = git.GitCommandError("ls-tree", 128, stderr="fatal: bad tree object") + + with pytest.raises(git.GitCommandError, match="ls-tree"): + _tree_entry_for_path(gitwd, "missing-ref", "test.go") + class TestRebases: def test_simple_dry_run(self, init_test_repositories, fake_github_provider, tmpdir): @@ -1603,6 +1611,122 @@ def test_excluded_skip_log_emitted( "Ensure caplog.set_level(logging.INFO) is set." ) + def test_real_upstream_merge_resolved_to_upstream_is_not_treated_as_synthetic_marker( + self, init_test_repositories, fake_github_provider, tmpdir + ): + source, rebase, dest = init_test_repositories + CommitBuilder(source).add_file("baz.txt", "upstream v2\n").commit("second upstream commit") + + first_run_dir = os.path.join(tmpdir, "first_run") + os.makedirs(first_run_dir) + assert cli.rebasebot_run( + _make_rebase_args(source, rebase, dest, first_run_dir), + slack_webhook=None, + github_app_wrapper=fake_github_provider, + ) + + dest_repo = _merge_rebase_branch_into_dest(dest, first_run_dir, "real_upstream_remote") + CommitBuilder(dest).update_file( + "test.go", + """package main +func main() { + println("downstream override") +} +""", + ).commit("downstream tweak before upstream sync") + CommitBuilder(source).update_file( + "test.go", + """package main +func main() { + println("upstream sync") +} +""", + ).commit("third upstream commit") + + remote_name = "manual_upstream_source" + if remote_name not in [remote.name for remote in dest_repo.remotes]: + dest_repo.create_remote(remote_name, source.url) + dest_repo.remotes[remote_name].fetch(source.branch) + dest_repo.git.checkout(dest.branch) + try: + dest_repo.git.merge(f"{remote_name}/{source.branch}", "--no-ff", "--no-commit") + except git.GitCommandError: + pass + dest_repo.git.checkout("--theirs", "test.go") + dest_repo.git.add("test.go") + dest_repo.git.rm("--force", "another_file.go") + dest_repo.git.commit("-m", "Manual upstream sync resolved fully to upstream") + manual_merge_sha = dest_repo.head.commit.hexsha + assert dest_repo.head.commit.tree.hexsha == dest_repo.head.commit.parents[1].tree.hexsha + + CommitBuilder(source).add_file("qux.txt", "upstream v3\n").commit("fourth upstream commit") + + second_run_dir = os.path.join(tmpdir, "second_run") + os.makedirs(second_run_dir) + assert cli.rebasebot_run( + _make_rebase_args(source, rebase, dest, second_run_dir), + slack_webhook=None, + github_app_wrapper=fake_github_provider, + ) + + expected_recovery_msg = ( + f"UPSTREAM: : Recover merge-only delta from merge {manual_merge_sha[:12]}" + ) + working_repo = Repo(second_run_dir) + commit_summaries = [commit.summary for commit in working_repo.iter_commits("rebase")] + assert expected_recovery_msg in commit_summaries, ( + "A real upstream merge that resolved fully to the upstream tree must still be " + "treated as a user-authored merge and recovered on later rebases.\n" + f"Got commits: {commit_summaries!r}" + ) + assert "another_file.go" not in working_repo.head.commit.tree + assert working_repo.git.show("HEAD:test.go") == Repo(source.url).git.show(f"{source.branch}:test.go") + + @patch("rebasebot.bot.subprocess.run") + def test_merge_only_delta_detection_failure_requires_manual_intervention( + self, mocked_subprocess_run, init_test_repositories, fake_github_provider, tmpdir + ): + source, rebase, dest = init_test_repositories + CommitBuilder(source).add_file("baz.txt", "upstream v2").commit("second upstream commit") + + first_run_dir = os.path.join(tmpdir, "first_run") + os.makedirs(first_run_dir) + assert cli.rebasebot_run( + _make_rebase_args(source, rebase, dest, first_run_dir), + slack_webhook=None, + github_app_wrapper=fake_github_provider, + ) + dest_repo = _merge_rebase_branch_into_dest(dest, first_run_dir, "failed_detection_remote") + + dest_repo.git.checkout("-b", "feature_branch") + with open(os.path.join(dest.url, "feature.txt"), "x", encoding="utf8") as fh: + fh.write("feature content\n") + dest_repo.git.add("feature.txt") + dest_repo.git.commit("-m", "add feature file") + + dest_repo.git.checkout(dest.branch) + dest_repo.git.merge("feature_branch", "--no-ff", "--no-commit") + with open(os.path.join(dest.url, "merge_resolution.txt"), "w", encoding="utf8") as fh: + fh.write("manually resolved\n") + dest_repo.git.add("merge_resolution.txt") + dest_repo.git.commit("-m", "Manual merge: feature into main with merge-only resolution") + + CommitBuilder(source).add_file("qux.txt", "upstream v3").commit("third upstream commit") + + mocked_subprocess_run.return_value = MagicMock(stdout="", returncode=0, stderr="") + + second_run_dir = os.path.join(tmpdir, "second_run") + os.makedirs(second_run_dir) + result = cli.rebasebot_run( + _make_rebase_args(source, rebase, dest, second_run_dir), + slack_webhook=None, + github_app_wrapper=fake_github_provider, + ) + assert result is False, ( + "If merge-only delta detection cannot produce a baseline for an eligible merge, " + "the rebase must fail closed instead of warning and continuing." + ) + def test_manual_intervention_warning_logged_on_apply_failure(self, caplog): """ Operator visibility: a WARNING log must be emitted specifically for the From d96f8f2a11e0abcdbe8b3d61c71c295d1965da60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Radek=20Ma=C5=88=C3=A1k?= Date: Thu, 25 Jun 2026 21:16:27 +0200 Subject: [PATCH 07/10] rebase: narrow synthetic merge detection Require the bot's configured git identity before treating a merge as RebaseBot's internal synthetic marker, and fail merge-only delta detection when merge-tree exits fatally. This preserves real upstream-sync merges that resolve to upstream and stops dry runs from silently continuing without a recoverable baseline. --- rebasebot/bot.py | 20 ++++++++++++++++++++ tests/test_rebases.py | 23 ++++++++++++++++++----- 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/rebasebot/bot.py b/rebasebot/bot.py index 9185ecc..a7f784f 100755 --- a/rebasebot/bot.py +++ b/rebasebot/bot.py @@ -156,6 +156,15 @@ def _tree_entry_for_path(gitwd: git.Repo, ref: str, path: str) -> str: return gitwd.git.ls_tree(ref, "--", path).strip() +def _is_commit_authored_by_current_git_user(commit: Commit, gitwd: git.Repo) -> bool: + try: + git_email = gitwd.config_reader().get_value("user", "email") + except Exception: # pragma: no cover - config lookup failures fall back to legacy heuristics + return False + + return commit.author.email == git_email and commit.committer.email == git_email + + def _is_rebasebot_synthetic_merge( merge: Commit, gitwd: git.Repo, @@ -176,6 +185,12 @@ def _is_rebasebot_synthetic_merge( if merge.summary != _synthetic_rebase_merge_message(source.branch, dest.branch): return False + # RebaseBot creates this synthetic merge locally with its configured git identity. + # A real upstream-sync merge can have the same tree shape and summary, so require + # bot authorship before treating it as an internal marker. + if not _is_commit_authored_by_current_git_user(merge, gitwd): + return False + upstream_parent = _find_source_parent_commit(parents, gitwd) if upstream_parent is None: return False @@ -571,6 +586,11 @@ def _find_merge_only_deltas(gitwd: git.Repo, source: GitHubBranch, dest: GitHubB ) # merge-tree exits 0 for clean merge, 1 for conflicts. # Either way the tree SHA is on the first line of stdout. + if proc.returncode not in (0, 1): + raise RepoException( + f"Failed to determine merge-only delta baseline for merge {sha[:12]}: " + f"git merge-tree exited {proc.returncode}: {proc.stderr.strip() or 'no stderr'}" + ) auto_tree = proc.stdout.strip().split("\n")[0].strip() if proc.stdout.strip() else "" if not auto_tree: raise RepoException( diff --git a/tests/test_rebases.py b/tests/test_rebases.py index 1b42486..cbaf1e8 100644 --- a/tests/test_rebases.py +++ b/tests/test_rebases.py @@ -1647,7 +1647,7 @@ def test_real_upstream_merge_resolved_to_upstream_is_not_treated_as_synthetic_ma if remote_name not in [remote.name for remote in dest_repo.remotes]: dest_repo.create_remote(remote_name, source.url) dest_repo.remotes[remote_name].fetch(source.branch) - dest_repo.git.checkout(dest.branch) + dest_repo.git.checkout("-b", "manual_upstream_sync_branch") try: dest_repo.git.merge(f"{remote_name}/{source.branch}", "--no-ff", "--no-commit") except git.GitCommandError: @@ -1655,9 +1655,18 @@ def test_real_upstream_merge_resolved_to_upstream_is_not_treated_as_synthetic_ma dest_repo.git.checkout("--theirs", "test.go") dest_repo.git.add("test.go") dest_repo.git.rm("--force", "another_file.go") - dest_repo.git.commit("-m", "Manual upstream sync resolved fully to upstream") + dest_repo.git.commit("-m", "merge upstream/main into main") manual_merge_sha = dest_repo.head.commit.hexsha assert dest_repo.head.commit.tree.hexsha == dest_repo.head.commit.parents[1].tree.hexsha + assert dest_repo.head.commit.author.email == "dest_author@dest.org" + + dest_repo.git.checkout(dest.branch) + dest_repo.git.merge( + "manual_upstream_sync_branch", + "--no-ff", + "-m", + "Merge manual upstream sync branch", + ) CommitBuilder(source).add_file("qux.txt", "upstream v3\n").commit("fourth upstream commit") @@ -1713,7 +1722,11 @@ def test_merge_only_delta_detection_failure_requires_manual_intervention( CommitBuilder(source).add_file("qux.txt", "upstream v3").commit("third upstream commit") - mocked_subprocess_run.return_value = MagicMock(stdout="", returncode=0, stderr="") + mocked_subprocess_run.return_value = MagicMock( + stdout=f"{dest_repo.head.commit.tree.hexsha}\nfatal: merge-tree exploded", + returncode=128, + stderr="fatal: merge-tree exploded", + ) second_run_dir = os.path.join(tmpdir, "second_run") os.makedirs(second_run_dir) @@ -1723,8 +1736,8 @@ def test_merge_only_delta_detection_failure_requires_manual_intervention( github_app_wrapper=fake_github_provider, ) assert result is False, ( - "If merge-only delta detection cannot produce a baseline for an eligible merge, " - "the rebase must fail closed instead of warning and continuing." + "If merge-tree fatally fails for an otherwise eligible merge, the rebase must fail " + "closed even when stdout starts with a tree-looking line." ) def test_manual_intervention_warning_logged_on_apply_failure(self, caplog): From 5e7d09b466bb168bb840de01c7783b56efad95d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Radek=20Ma=C5=88=C3=A1k?= Date: Wed, 1 Jul 2026 11:04:12 +0200 Subject: [PATCH 08/10] rebase: fail closed on partial merge-only carry recovery Reject recovered merge-only carries once rebased paths have diverged past the original merge baseline, and surface deleted-path rm failures instead of silently partial-applying the carry. Reuse the shared rebase arg builder in the merge-only regression tests so these scenarios stay aligned with the dry-run defaults. --- rebasebot/bot.py | 105 +++++++++++++++++++-------- tests/test_rebases.py | 165 +++++++++++++++++++++--------------------- 2 files changed, 160 insertions(+), 110 deletions(-) diff --git a/rebasebot/bot.py b/rebasebot/bot.py index a7f784f..9776137 100755 --- a/rebasebot/bot.py +++ b/rebasebot/bot.py @@ -62,6 +62,41 @@ def _synthetic_rebase_merge_message(source_branch: str, dest_branch: str) -> str return f"merge upstream/{source_branch} into {dest_branch}" +def _merge_tree_write_tree(gitwd: git.Repo, merge_commit: Commit) -> str: + """Return the synthetic auto-merge tree SHA for a two-parent merge commit.""" + parents = list(merge_commit.parents) + short_sha = merge_commit.hexsha[:12] + if len(parents) != _MERGE_COMMIT_PARENT_COUNT: + raise RepoException(f"Failed to determine merge-only delta baseline for merge {short_sha}: expected 2 parents") + + p1, p2 = parents[0].hexsha, parents[1].hexsha + try: + proc = subprocess.run( + ["git", "merge-tree", "--write-tree", p1, p2], + capture_output=True, + text=True, + cwd=gitwd.working_dir, + ) + # merge-tree exits 0 for clean merge, 1 for conflicts. + # Either way the tree SHA is on the first line of stdout. + if proc.returncode not in (0, 1): + raise RepoException( + f"Failed to determine merge-only delta baseline for merge {short_sha}: " + f"git merge-tree exited {proc.returncode}: {proc.stderr.strip() or 'no stderr'}" + ) + auto_tree = proc.stdout.strip().split("\n")[0].strip() if proc.stdout.strip() else "" + if not auto_tree: + raise RepoException( + f"Failed to determine merge-only delta baseline for merge {short_sha}: " + f"git merge-tree produced no tree SHA (exit {proc.returncode})" + ) + return auto_tree + except OSError as ex: + raise RepoException( + f"Failed to determine merge-only delta baseline for merge {short_sha}: {ex}" + ) from ex + + def _message_slack(webhook_url: str, msg: str) -> None: """Send a message to Slack via a webhook if one is configured.""" if webhook_url is None: @@ -360,7 +395,11 @@ def _identify_downstream_commit_phases( "Phase 1 sanity check failed: commits from the rebase PR range are not downstream commits:\n" f"{extras_text}" ) - logging.info("Phase 1 - rebase PR carries (%d commits):\n%s", len(phase1_lines), "\n".join(phase1_lines)) + logging.info( + "Phase 1 - rebase PR carries (%d commits):\n%s", + len(phase1_lines), + "\n".join(phase1_lines), + ) else: logging.info("Could not find rebase PR merge on dest, skipping phase 1") @@ -576,31 +615,7 @@ def _find_merge_only_deltas(gitwd: git.Repo, source: GitHubBranch, dest: GitHubB continue # Compute auto-merge tree baseline via git merge-tree --write-tree. - p1, p2 = parents[0].hexsha, parents[1].hexsha - try: - proc = subprocess.run( - ["git", "merge-tree", "--write-tree", p1, p2], - capture_output=True, - text=True, - cwd=gitwd.working_dir, - ) - # merge-tree exits 0 for clean merge, 1 for conflicts. - # Either way the tree SHA is on the first line of stdout. - if proc.returncode not in (0, 1): - raise RepoException( - f"Failed to determine merge-only delta baseline for merge {sha[:12]}: " - f"git merge-tree exited {proc.returncode}: {proc.stderr.strip() or 'no stderr'}" - ) - auto_tree = proc.stdout.strip().split("\n")[0].strip() if proc.stdout.strip() else "" - if not auto_tree: - raise RepoException( - f"Failed to determine merge-only delta baseline for merge {sha[:12]}: " - f"git merge-tree produced no tree SHA (exit {proc.returncode})" - ) - except OSError as ex: - raise RepoException( - f"Failed to determine merge-only delta baseline for merge {sha[:12]}: {ex}" - ) from ex + auto_tree = _merge_tree_write_tree(gitwd, merge) if auto_tree == merge.tree.hexsha: logging.info("No merge-only delta in merge %s", sha[:12]) @@ -659,6 +674,8 @@ def _apply_merge_only_delta(gitwd: git.Repo, merge_commit: Commit, diff_output: added_or_modified.append(path) # Compare full tree entries so mode-only and other non-text changes are not mistaken for no-ops. + added_or_modified = list(dict.fromkeys(added_or_modified)) + deleted = list(dict.fromkeys(deleted)) affected_paths = list(dict.fromkeys([*added_or_modified, *deleted])) has_change = any(_tree_entry_for_path(gitwd, "HEAD", path) != _tree_entry_for_path(gitwd, sha, path) for path in affected_paths) @@ -666,6 +683,29 @@ def _apply_merge_only_delta(gitwd: git.Repo, merge_commit: Commit, diff_output: logging.info("Merge-only delta from %s is a no-op after replay; skipping", short_sha) return + baseline_tree = _merge_tree_write_tree(gitwd, merge_commit) + parent_refs = [parent.hexsha for parent in merge_commit.parents] + conflicting_paths = [] + for path in affected_paths: + head_entry = _tree_entry_for_path(gitwd, "HEAD", path) + baseline_entry = _tree_entry_for_path(gitwd, baseline_tree, path) + target_entry = _tree_entry_for_path(gitwd, sha, path) + parent_entries = tuple(_tree_entry_for_path(gitwd, parent_ref, path) for parent_ref in parent_refs) + if head_entry not in (baseline_entry, target_entry, *parent_entries): + conflicting_paths.append(path) + + if conflicting_paths: + conflicted = ", ".join(conflicting_paths) + logging.warning( + "Cannot apply merge-only delta from merge %s cleanly; paths diverged from replay baseline: %s", + short_sha, + conflicted, + ) + raise RepoException( + f"Cannot apply merge-only delta from merge {short_sha} cleanly; " + f"rebased paths diverged from merge baseline: {conflicted}" + ) + # Restore added/modified paths to their state in the merge commit's tree. if added_or_modified: try: @@ -682,9 +722,16 @@ def _apply_merge_only_delta(gitwd: git.Repo, merge_commit: Commit, diff_output: # Remove paths that were deleted in the merge commit relative to the auto-merge baseline. for path in deleted: try: - gitwd.git.rm("--force", path) - except git.GitCommandError: - pass # Already absent; ignore. + gitwd.git.rm("--force", "--ignore-unmatch", "--", path) + except git.GitCommandError as ex: + logging.warning( + "Cannot remove merge-only delta path %s from merge %s cleanly; manual intervention required", + path, + short_sha, + ) + raise RepoException( + f"Failed to remove merge-only delta path {path} from merge {short_sha}: {ex}" + ) from ex # Verify there are actually staged changes (guard against no-op after checkout). try: diff --git a/tests/test_rebases.py b/tests/test_rebases.py index cbaf1e8..9029f2b 100644 --- a/tests/test_rebases.py +++ b/tests/test_rebases.py @@ -940,33 +940,7 @@ def test_later_run_merge_only_delta_preserved(self, init_test_repositories, fake first_run_dir = os.path.join(tmpdir, "first_run") os.makedirs(first_run_dir) - def _make_args(src, rbs, dst, wdir): - a = MagicMock() - a.source = src - a.source_repo = None - a.dest = dst - a.rebase = rbs - a.working_dir = wdir - a.git_username = "test_rebasebot" - a.git_email = "test@rebasebot.ocp" - a.tag_policy = "soft" - a.bot_emails = [] - a.exclude_commits = [] - a.update_go_modules = False - a.conflict_policy = "auto" - a.ignore_manual_label = False - a.dry_run = True - a.always_run_hooks = False - a.title_prefix = "" - a.pre_rebase_hook = None - a.post_rebase_hook = None - a.pre_carry_commit_hook = None - a.pre_push_rebase_branch_hook = None - a.pre_create_pr_hook = None - a.source_ref_hook = None - return a - - args1 = _make_args(source, rebase, dest, first_run_dir) + args1 = _make_rebase_args(source, rebase, dest, first_run_dir) result1 = cli.rebasebot_run(args1, slack_webhook=None, github_app_wrapper=fake_github_provider) assert result1, "First rebase run should succeed" @@ -1006,7 +980,7 @@ def _make_args(src, rbs, dst, wdir): # ── Second dry-run rebase (the "later run") ───────────────────────────── second_run_dir = os.path.join(tmpdir, "second_run") os.makedirs(second_run_dir) - args2 = _make_args(source, rebase, dest, second_run_dir) + args2 = _make_rebase_args(source, rebase, dest, second_run_dir) result2 = cli.rebasebot_run(args2, slack_webhook=None, github_app_wrapper=fake_github_provider) assert result2, "Second (later) rebase run should succeed" @@ -1072,29 +1046,7 @@ def test_first_run_legacy_merge_only_delta_preserved(self, init_test_repositorie legacy_merge_sha = dest_repo.head.commit.hexsha # Run the first dry-run rebase (no prior rebasebot marker on dest). - args = MagicMock() - args.source = source - args.source_repo = None - args.dest = dest - args.rebase = rebase - args.working_dir = tmpdir - args.git_username = "test_rebasebot" - args.git_email = "test@rebasebot.ocp" - args.tag_policy = "soft" - args.bot_emails = [] - args.exclude_commits = [] - args.update_go_modules = False - args.conflict_policy = "auto" - args.ignore_manual_label = False - args.dry_run = True - args.always_run_hooks = False - args.title_prefix = "" - args.pre_rebase_hook = None - args.post_rebase_hook = None - args.pre_carry_commit_hook = None - args.pre_push_rebase_branch_hook = None - args.pre_create_pr_hook = None - args.source_ref_hook = None + args = _make_rebase_args(source, rebase, dest, tmpdir) result = cli.rebasebot_run(args, slack_webhook=None, github_app_wrapper=fake_github_provider) assert result, "First-run rebase should succeed" @@ -1135,33 +1087,7 @@ def test_later_run_merge_only_delta_is_replayed_before_later_downstream_commits( first_run_dir = os.path.join(tmpdir, "first_run") os.makedirs(first_run_dir) - def _make_args(src, rbs, dst, wdir): - a = MagicMock() - a.source = src - a.source_repo = None - a.dest = dst - a.rebase = rbs - a.working_dir = wdir - a.git_username = "test_rebasebot" - a.git_email = "test@rebasebot.ocp" - a.tag_policy = "soft" - a.bot_emails = [] - a.exclude_commits = [] - a.update_go_modules = False - a.conflict_policy = "auto" - a.ignore_manual_label = False - a.dry_run = True - a.always_run_hooks = False - a.title_prefix = "" - a.pre_rebase_hook = None - a.post_rebase_hook = None - a.pre_carry_commit_hook = None - a.pre_push_rebase_branch_hook = None - a.pre_create_pr_hook = None - a.source_ref_hook = None - return a - - args1 = _make_args(source, rebase, dest, first_run_dir) + args1 = _make_rebase_args(source, rebase, dest, first_run_dir) assert cli.rebasebot_run(args1, slack_webhook=None, github_app_wrapper=fake_github_provider) dest_repo = Repo(dest.url) @@ -1197,7 +1123,7 @@ def _make_args(src, rbs, dst, wdir): second_run_dir = os.path.join(tmpdir, "second_run") os.makedirs(second_run_dir) - args2 = _make_args(source, rebase, dest, second_run_dir) + args2 = _make_rebase_args(source, rebase, dest, second_run_dir) assert cli.rebasebot_run(args2, slack_webhook=None, github_app_wrapper=fake_github_provider) second_working = Repo(second_run_dir) @@ -1430,14 +1356,54 @@ def test_replay_time_no_op_merge_only_delta_is_skipped( ) assert os.path.exists(os.path.join(second_run_dir, "merge_resolution.txt")) - def test_recovery_requires_manual_intervention_when_synthetic_carry_cannot_apply_cleanly(self): + @patch("rebasebot.bot.subprocess.run") + def test_recovery_requires_manual_intervention_when_rebased_head_diverged_from_merge_baseline( + self, merge_tree_run + ): + merge_tree_run.return_value = MagicMock(returncode=0, stdout="baseline-tree\n", stderr="") + + gitwd = MagicMock() + merge_commit = MagicMock() + merge_commit.hexsha = "1234567890abcdef1234567890abcdef12345678" + merge_commit.parents = [MagicMock(hexsha="parent-one"), MagicMock(hexsha="parent-two")] + + gitwd.git.ls_tree.side_effect = [ + "100644 blob headbead\tmerge_resolution.txt", + "100644 blob target123\tmerge_resolution.txt", + "100644 blob headbead\tmerge_resolution.txt", + "100644 blob basecafe\tmerge_resolution.txt", + "100644 blob target123\tmerge_resolution.txt", + "100644 blob parent111\tmerge_resolution.txt", + "100644 blob parent222\tmerge_resolution.txt", + ] + + with pytest.raises(RepoException, match="Cannot apply merge-only delta from merge 1234567890ab cleanly"): + _apply_merge_only_delta( + gitwd, + merge_commit, + ":000000 100644 0000000 1111111 A\tmerge_resolution.txt", + ) + + gitwd.git.checkout.assert_not_called() + gitwd.git.rm.assert_not_called() + + @patch("rebasebot.bot.subprocess.run") + def test_recovery_requires_manual_intervention_when_synthetic_carry_cannot_apply_cleanly(self, merge_tree_run): + merge_tree_run.return_value = MagicMock(returncode=0, stdout="baseline-tree\n", stderr="") + gitwd = MagicMock() merge_commit = MagicMock() merge_commit.hexsha = "1234567890abcdef1234567890abcdef12345678" + merge_commit.parents = [MagicMock(hexsha="parent-one"), MagicMock(hexsha="parent-two")] gitwd.git.ls_tree.side_effect = [ "100644 blob deadbeef\tmerge_resolution.txt", "100644 blob feedface\tmerge_resolution.txt", + "100644 blob deadbeef\tmerge_resolution.txt", + "100644 blob deadbeef\tmerge_resolution.txt", + "100644 blob feedface\tmerge_resolution.txt", + "100644 blob deadbeef\tmerge_resolution.txt", + "100644 blob parent222\tmerge_resolution.txt", ] gitwd.git.checkout.side_effect = git.GitCommandError( "checkout", @@ -1452,6 +1418,35 @@ def test_recovery_requires_manual_intervention_when_synthetic_carry_cannot_apply ":000000 100644 0000000 1111111 A\tmerge_resolution.txt", ) + @patch("rebasebot.bot.subprocess.run") + def test_deleted_merge_only_paths_use_pathspec_separator_and_raise_on_rm_failures(self, merge_tree_run): + merge_tree_run.return_value = MagicMock(returncode=0, stdout="baseline-tree\n", stderr="") + + gitwd = MagicMock() + merge_commit = MagicMock() + merge_commit.hexsha = "1234567890abcdef1234567890abcdef12345678" + merge_commit.parents = [MagicMock(hexsha="parent-one"), MagicMock(hexsha="parent-two")] + + gitwd.git.ls_tree.side_effect = [ + "100644 blob deadbeef\t-dash-path", + "", + "100644 blob deadbeef\t-dash-path", + "100644 blob deadbeef\t-dash-path", + "", + "100644 blob deadbeef\t-dash-path", + "", + ] + gitwd.git.rm.side_effect = git.GitCommandError("rm", 1, stderr="fatal: permission denied") + + with pytest.raises(RepoException, match="Failed to remove merge-only delta path -dash-path from merge 1234567890ab"): + _apply_merge_only_delta( + gitwd, + merge_commit, + ":100644 000000 deadbeef 0000000 D\t-dash-path", + ) + + gitwd.git.rm.assert_called_once_with("--force", "--ignore-unmatch", "--", "-dash-path") + def test_detection_and_recovery_logs_emitted( self, init_test_repositories, fake_github_provider, tmpdir, caplog ): @@ -1740,7 +1735,8 @@ def test_merge_only_delta_detection_failure_requires_manual_intervention( "closed even when stdout starts with a tree-looking line." ) - def test_manual_intervention_warning_logged_on_apply_failure(self, caplog): + @patch("rebasebot.bot.subprocess.run") + def test_manual_intervention_warning_logged_on_apply_failure(self, merge_tree_run, caplog): """ Operator visibility: a WARNING log must be emitted specifically for the merge-only delta recovery failure case before the RepoException is raised. @@ -1749,13 +1745,20 @@ def test_manual_intervention_warning_logged_on_apply_failure(self, caplog): the precise merge that required manual intervention without needing to parse the generic outer error. """ + merge_tree_run.return_value = MagicMock(returncode=0, stdout="baseline-tree\n", stderr="") gitwd = MagicMock() merge_commit = MagicMock() merge_commit.hexsha = "abcdef012345abcdef012345abcdef0123456789" + merge_commit.parents = [MagicMock(hexsha="parent-one"), MagicMock(hexsha="parent-two")] gitwd.git.ls_tree.side_effect = [ "100644 blob deadbeef\tconflict_file.txt", "100644 blob feedface\tconflict_file.txt", + "100644 blob deadbeef\tconflict_file.txt", + "100644 blob deadbeef\tconflict_file.txt", + "100644 blob feedface\tconflict_file.txt", + "100644 blob deadbeef\tconflict_file.txt", + "100644 blob parent222\tconflict_file.txt", ] gitwd.git.checkout.side_effect = git.GitCommandError( "checkout", From cf0278a7ff396138b8866fc4355cceebb6edfa85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Radek=20Ma=C5=88=C3=A1k?= Date: Fri, 3 Jul 2026 11:51:36 +0200 Subject: [PATCH 09/10] fix: restrict bot email normalization to GitHub noreply Only normalize numeric-prefix plus-addresses for GitHub noreply commits so real email addresses are not collapsed during bot squash matching. Update the rebase regression coverage to keep the noreply case and the non-GitHub preservation case explicit. --- rebasebot/bot.py | 4 ++++ tests/test_rebases.py | 24 ++++++++++++++++++------ 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/rebasebot/bot.py b/rebasebot/bot.py index 9b607cb..1c014d4 100755 --- a/rebasebot/bot.py +++ b/rebasebot/bot.py @@ -189,6 +189,10 @@ def _normalize_bot_email(email: str) -> str: if not sep: return email + domain = domain.lower() + if domain != "users.noreply.github.com": + return email + prefix, plus, rest = local.partition("+") if plus and prefix.isdigit() and rest: return f"{rest}@{domain}" diff --git a/tests/test_rebases.py b/tests/test_rebases.py index 3f9dc6a..698a35d 100644 --- a/tests/test_rebases.py +++ b/tests/test_rebases.py @@ -15,6 +15,7 @@ _apply_merge_only_delta, _init_working_dir, _needs_rebase, + _normalize_bot_email, _prepare_rebase_branch, _tree_entry_for_path, ) @@ -151,6 +152,13 @@ def test_tree_entry_for_path_propagates_real_lookup_failures(self): with pytest.raises(git.GitCommandError, match="ls-tree"): _tree_entry_for_path(gitwd, "missing-ref", "test.go") + def test_normalize_bot_email_only_rewrites_github_noreply_addresses(self): + assert ( + _normalize_bot_email("12345+genbot@users.noreply.github.com") + == "genbot@users.noreply.github.com" + ) + assert _normalize_bot_email("12345+genbot@example.com") == "12345+genbot@example.com" + class TestRebases: def test_simple_dry_run(self, init_test_repositories, fake_github_provider, tmpdir): @@ -256,10 +264,10 @@ def test_squash_bot_email_normalization(self, init_test_repositories, fake_githu cb.commit("other upstream commit") with CommitBuilder(dest) as cb: cb.add_file("generated-test-genbot", "content") - cb.commit("commit #1 from genbot", committer_email="12345+genbot@example.com") + cb.commit("commit #1 from genbot", committer_email="12345+genbot@users.noreply.github.com") with CommitBuilder(dest) as cb: cb.add_file("generated-test-genbot-2", "content") - cb.commit("commit #2 from genbot", committer_email="12345+genbot@example.com") + cb.commit("commit #2 from genbot", committer_email="12345+genbot@users.noreply.github.com") with CommitBuilder(dest) as cb: cb.add_file("generated-test-ci", "content") cb.commit("commit #1 from ci nightly", committer_email="ci+nightly@example.com") @@ -276,7 +284,11 @@ def test_squash_bot_email_normalization(self, init_test_repositories, fake_githu args.git_username = "test_rebasebot" args.git_email = "test@rebasebot.ocp" args.tag_policy = "soft" - args.bot_emails = ["12345+genbot@example.com", "ci+nightly@example.com", "ops+nightly@example.com"] + args.bot_emails = [ + "12345+genbot@users.noreply.github.com", + "ci+nightly@example.com", + "ops+nightly@example.com", + ] args.exclude_commits = [] args.update_go_modules = False args.conflict_policy = "auto" @@ -295,15 +307,15 @@ def test_squash_bot_email_normalization(self, init_test_repositories, fake_githu == r""" * ', commit #1 from ops nightly' * ', commit #1 from ci nightly' -* ', commit #2 from genbot' +* ', commit #2 from genbot' * ', UPSTREAM: : our cool addition' * ', merge upstream/main into main' |\ | * ', other upstream commit' * | ', commit #1 from ops nightly' * | ', commit #1 from ci nightly' -* | ', commit #2 from genbot' -* | ', commit #1 from genbot' +* | ', commit #2 from genbot' +* | ', commit #1 from genbot' * | ', UPSTREAM: : our cool addition' |/ * ', Upstream commit' From 3915fc54c493977e13b959ad9a9e4a5048050976 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Radek=20Ma=C5=88=C3=A1k?= Date: Fri, 3 Jul 2026 13:01:52 +0200 Subject: [PATCH 10/10] rebase: simplify merge-only replay helpers Drop the unused downstream-commit wrapper and reuse the shared rebase-merge helper in the repetitive later-run tests so the merge-only coverage stays focused on replay behavior instead of setup duplication. --- rebasebot/bot.py | 6 ------ tests/test_rebases.py | 18 ++---------------- 2 files changed, 2 insertions(+), 22 deletions(-) diff --git a/rebasebot/bot.py b/rebasebot/bot.py index 1c014d4..ed73421 100755 --- a/rebasebot/bot.py +++ b/rebasebot/bot.py @@ -438,12 +438,6 @@ def _identify_downstream_commit_phases( return phase1_lines, phase2_lines, cutoff_commits -def _identify_downstream_commits(gitwd: git.Repo, source: GitHubBranch, dest: GitHubBranch) -> str: - phase1_lines, phase2_lines, _ = _identify_downstream_commit_phases(gitwd, source, dest) - downstream_commits = "\n".join([*phase1_lines, *phase2_lines]) - return downstream_commits - - def _detect_conflicting_files(gitwd: git.Repo, sha: str) -> set: """ Probe a cherry-pick without -Xtheirs to detect which files conflict. diff --git a/tests/test_rebases.py b/tests/test_rebases.py index 698a35d..d5d042a 100644 --- a/tests/test_rebases.py +++ b/tests/test_rebases.py @@ -1074,14 +1074,7 @@ def test_later_run_merge_only_delta_preserved(self, init_test_repositories, fake assert result1, "First rebase run should succeed" # ── Simulate merging the rebase PR into dest (--no-ff) ───────────────── - dest_repo = Repo(dest.url) - dest_repo.git.checkout(dest.branch) - - remote_name = "first_run_remote" - if remote_name not in [r.name for r in dest_repo.remotes]: - dest_repo.create_remote(remote_name, first_run_dir) - dest_repo.remotes[remote_name].fetch("rebase") - dest_repo.git.merge(f"{remote_name}/rebase", "--no-ff", "-m", "Merge rebase PR into main") + dest_repo = _merge_rebase_branch_into_dest(dest, first_run_dir, "first_run_remote") # ── Create a feature branch and merge it with merge-only content ──────── dest_repo.git.checkout("-b", "feature_branch") @@ -1219,14 +1212,7 @@ def test_later_run_merge_only_delta_is_replayed_before_later_downstream_commits( args1 = _make_rebase_args(source, rebase, dest, first_run_dir) assert cli.rebasebot_run(args1, slack_webhook=None, github_app_wrapper=fake_github_provider) - dest_repo = Repo(dest.url) - dest_repo.git.checkout(dest.branch) - - remote_name = "first_run_remote" - if remote_name not in [r.name for r in dest_repo.remotes]: - dest_repo.create_remote(remote_name, first_run_dir) - dest_repo.remotes[remote_name].fetch("rebase") - dest_repo.git.merge(f"{remote_name}/rebase", "--no-ff", "-m", "Merge rebase PR into main") + dest_repo = _merge_rebase_branch_into_dest(dest, first_run_dir, "first_run_remote") dest_repo.git.checkout("-b", "feature_branch") feature_file_path = os.path.join(dest.url, "feature.txt")