diff --git a/README.md b/README.md index 29386836..57a964af 100644 --- a/README.md +++ b/README.md @@ -48,3 +48,19 @@ file for more details about which credentials are required. Checkout the [`tox.ini`](./tox.ini) for all available commands to develop with bert-e. + +## Contributing a new check + +Every new check that blocks a merge **must** ship with a corresponding +`bypass_` privileged option. This is a hard requirement — without +a bypass, a false positive permanently blocks a legitimate PR with no +administrator escape hatch. + +Checklist for a new check: + +1. Add `bypass_(job)` to `bert_e/workflow/gitwaterflow/utils.py` +2. Register the option in `commands.py` `setup()` with `privileged=True` +3. Import and call `bypass_(job)` at the top of the check function +4. Mention the bypass in the error message template +5. Document the check and bypass in `bert_e/docs/USER_DOC.md` +6. Add a unit test that verifies the bypass skips the check diff --git a/bert_e/docs/USER_DOC.md b/bert_e/docs/USER_DOC.md index dc4f4fcd..664a6124 100644 --- a/bert_e/docs/USER_DOC.md +++ b/bert_e/docs/USER_DOC.md @@ -118,6 +118,7 @@ __Bert-E__. | bypass_jira_check | Bypass the Jira issue check | yes | no | bypass_peer_approval | Bypass the pull request peer's approval | yes | no | bypass_leader_approval | Bypass the pull request leader's approval | yes | no +| bypass_source_branch_lineage | Bypass the cross-branch contamination check | yes | no | create_pull_requests | Let __Bert-E__ create pull requests corresponding to integration branches | no | no | create_integration_branches | Request __Bert-E__ to create integration branches and move forward with the gitwaterflow | no | no | no_octopus | Prevent Wall-E from doing any octopus merge and use multiple consecutive merge instead | yes | no @@ -278,6 +279,21 @@ includes: _feature/..._, _bugfix/..._, _improvement/..._. ___ +**The source branch does not carry commits from a higher release line.** +__Bert-E__ verifies that the source branch has not been accidentally rebased +on a Bert-E integration commit (e.g. a `w/` branch) or another higher +development branch. Doing so would cause a silent fast-forward that merges +hundreds of unrelated commits into a maintenance branch. + +*__Bert-E__ sends message code 137 in case of non-conformance.* + +> This check can be bypassed by an admin with the +> __bypass_source_branch_lineage__ option. +> Use only when the detection is a confirmed false positive (e.g. a legitimate +> backport that shares history with a higher line). + +--- + **The prefix of the source branch is compatible with the destination branch.** __Bert-E__ prevents the merge of a feature in a maintenance branch (only bugfixes and improvements branches are accepted). @@ -490,6 +506,7 @@ to progress to the next step. message code | 122 | Unknown command | One of the participants asked __Bert-E__ to activate an option, or execute a command he doesn't know. Edit the corresponding message if it contains a typo. Delete it otherwise | 123 | Not authorized | One of the participants asked __Bert-E__ to activate a privileged option, or execute a privileged command, but doesn't have enough credentials to do so. Delete the corresponding command ask a __Bert-E__ administrator to run/set the desired command/option. | 134 | Not author | One of the participants asked __Bert-E__ to activate an authored option, but the participant is not the author of the pull request. +| 137 | Foreign commits in source branch | The source branch shares history with a higher release line. Rebase the branch directly on the target branch, or ask an administrator to set `bypass_source_branch_lineage` if this is a confirmed false positive. Queues ------ diff --git a/bert_e/exceptions.py b/bert_e/exceptions.py index ecdcef9d..f9654dad 100644 --- a/bert_e/exceptions.py +++ b/bert_e/exceptions.py @@ -276,6 +276,12 @@ class QueueBuildFailedMessage(TemplateException): template = "queue_build_failed.md" +class ForeignCommitsInSourceBranch(TemplateException): + code = 137 + template = "foreign_commits_in_source_branch.md" + status = "failure" + + # internal exceptions class UnableToSendEmail(InternalException): code = 201 diff --git a/bert_e/settings.py b/bert_e/settings.py index d00cf318..66b68434 100644 --- a/bert_e/settings.py +++ b/bert_e/settings.py @@ -92,6 +92,7 @@ class PrAuthorsOptions(fields.Dict): 'bypass_incompatible_branch', 'bypass_peer_approval', 'bypass_leader_approval', + 'bypass_source_branch_lineage', ] def serialize(self, value, attr=None, obj=None, **kwargs): diff --git a/bert_e/templates/foreign_commits_in_source_branch.md b/bert_e/templates/foreign_commits_in_source_branch.md new file mode 100644 index 00000000..d89e34f4 --- /dev/null +++ b/bert_e/templates/foreign_commits_in_source_branch.md @@ -0,0 +1,46 @@ +{% extends "message.md" %} + +{% block title -%} +Foreign commits detected in source branch +{% endblock %} + +{% block message %} +The source branch `{{ src_branch }}` shares history with the following +release line(s), which are not ancestors of `{{ dst_branch }}`: + +{% for branch in foreign_branches %} +- `{{ branch }}` +{% endfor %} + +This typically happens when the feature branch was accidentally based on +commits from a higher release line rather than directly on `{{ dst_branch }}`. +Common causes include: + +- Rebasing on a Bert-E integration branch (e.g. a `w/` branch) instead of + directly on `{{ dst_branch }}` +- Branching from a higher development branch instead of `{{ dst_branch }}` +- Merging a higher development branch into the feature branch + +**How to fix** + +Create a new branch directly from `{{ dst_branch }}` and cherry-pick your +changes onto it: + +``` +git checkout -b origin/{{ dst_branch }} +git cherry-pick +``` + +Then open a new pull request from that branch. + +**If this is a false positive** + +If your branch is a legitimate backport and was previously merged into one +of the branches above before being extended with new commits, an +administrator can bypass this check with: + +``` +@bert-e bypass_source_branch_lineage +``` + +{% endblock %} diff --git a/bert_e/tests/unit/conftest.py b/bert_e/tests/unit/conftest.py index b6eded84..af6a20ec 100644 --- a/bert_e/tests/unit/conftest.py +++ b/bert_e/tests/unit/conftest.py @@ -1,12 +1,30 @@ """Unit tests fixtures.""" import os +import sys from os.path import abspath +from types import ModuleType +from unittest.mock import MagicMock + import pytest from bert_e.settings import setup_settings +def _install_jira_stub(): + """Stub jira==2.0.0 which imports `imghdr` removed in Python 3.13.""" + if 'jira' not in sys.modules: + _stub = ModuleType('jira') + _stub.JIRA = MagicMock() + _stub.exceptions = ModuleType('jira.exceptions') + _stub.exceptions.JIRAError = Exception + sys.modules['jira'] = _stub + sys.modules['jira.exceptions'] = _stub.exceptions + + +_install_jira_stub() + + @pytest.fixture def settings(): """Simple settings fixture.""" diff --git a/bert_e/tests/unit/test_check_source_branch_lineage.py b/bert_e/tests/unit/test_check_source_branch_lineage.py new file mode 100644 index 00000000..ef3618d9 --- /dev/null +++ b/bert_e/tests/unit/test_check_source_branch_lineage.py @@ -0,0 +1,486 @@ +"""Unit tests for check_source_branch_lineage.""" +from types import SimpleNamespace +from unittest.mock import MagicMock + +import pytest + +from bert_e import exceptions as messages +from bert_e.lib.simplecmd import CommandError +from bert_e.workflow.gitwaterflow import check_source_branch_lineage + + +def _make_branch(name, latest_commit, ancestor_of=None): + """Build a fake branch stub. + + ancestor_of: set of commit shas that this branch considers as ancestors. + """ + b = SimpleNamespace(name=name) + b.get_latest_commit = MagicMock(return_value=latest_commit) + ancestor_of = ancestor_of or set() + b.includes_commit = MagicMock( + side_effect=lambda commit: commit in ancestor_of + ) + return b + + +def _make_job(src_name, dst_name, dst_ancestors, cascade_branches, + merge_base_map, bypass=False): + """Return a minimal job stub for check_source_branch_lineage. + + merge_base_map: dict[(src_sha, higher_tip)] -> merge-base sha. + The implementation passes pre-resolved SHAs to git merge-base, so keys + must be commit SHAs, not branch names. + Missing keys raise CommandError (no common ancestor). + bypass: if True, sets bypass_source_branch_lineage on settings. + """ + def fake_cmd(template, *args): + key = (args[0], args[1]) + result = merge_base_map.get(key) + if result is None: + raise CommandError('no common ancestor') + return result + '\n' + + repo = SimpleNamespace(cmd=fake_cmd) + src = SimpleNamespace( + name=src_name, + get_latest_commit=MagicMock(return_value='src-tip'), + ) + dst = _make_branch(dst_name, 'dst-tip', ancestor_of=dst_ancestors) + cascade = SimpleNamespace(dst_branches=cascade_branches) + + git = SimpleNamespace(src_branch=src, dst_branch=dst, cascade=cascade, + repo=repo) + settings = SimpleNamespace(bypass_source_branch_lineage=bypass) + return SimpleNamespace(git=git, active_options={}, settings=settings, + author_bypass={}) + + +class TestCheckSourceBranchLineageClean: + """Source branch is cleanly based on the target — no error expected.""" + + def test_no_higher_branches(self): + """Single branch in cascade: nothing to compare against.""" + dst = _make_branch('development/4.3', 'dst-tip', ancestor_of=set()) + job = _make_job( + src_name='feature/BERTE-612-something', + dst_name='development/4.3', + dst_ancestors=set(), + cascade_branches=[dst], + merge_base_map={}, + ) + check_source_branch_lineage(job) # must not raise + + def test_higher_branch_but_merge_base_in_dst(self): + """Merge-base with the higher branch is already in dst — clean.""" + dst_ancestors = {'common-base'} + dst = _make_branch('development/4.3', 'dst-tip', + ancestor_of=dst_ancestors) + higher = _make_branch('development/4', 'higher-tip', + ancestor_of=set()) + + job = _make_job( + src_name='feature/BERTE-612-something', + dst_name='development/4.3', + dst_ancestors=dst_ancestors, + cascade_branches=[dst, higher], + # keys are (src_sha, higher_tip): 'src-tip' and 'higher-tip' + merge_base_map={ + ('src-tip', 'higher-tip'): 'common-base' + }, + ) + check_source_branch_lineage(job) # must not raise + + def test_merge_base_command_raises(self): + """If git merge-base fails for a higher branch, skip it silently.""" + dst = _make_branch('development/4.3', 'dst-tip', ancestor_of=set()) + higher = _make_branch('development/4', 'higher-tip', ancestor_of=set()) + + job = _make_job( + src_name='feature/BERTE-612-something', + dst_name='development/4.3', + dst_ancestors=set(), + cascade_branches=[dst, higher], + merge_base_map={}, # missing key → raises CommandError + ) + check_source_branch_lineage(job) # must not raise + + def test_two_higher_branches_first_clean_second_clean(self): + """Loop iterates all higher branches; no raise when both are clean.""" + dst_ancestors = {'base'} + dst = _make_branch('development/4.3', 'dst-tip', + ancestor_of=dst_ancestors) + higher1 = _make_branch('development/5.1', 'h1-tip', ancestor_of=set()) + higher2 = _make_branch('development/10.0', 'h2-tip', ancestor_of=set()) + + job = _make_job( + src_name='feature/BERTE-612-something', + dst_name='development/4.3', + dst_ancestors=dst_ancestors, + cascade_branches=[dst, higher1, higher2], + merge_base_map={ + ('src-tip', 'h1-tip'): 'base', + ('src-tip', 'h2-tip'): 'base', + }, + ) + check_source_branch_lineage(job) # must not raise + + def test_higher_get_latest_commit_raises_is_skipped(self): + """If higher.get_latest_commit() raises CommandError, skip it.""" + dst = _make_branch('development/4.3', 'dst-tip', ancestor_of=set()) + higher = _make_branch('development/4', 'higher-tip', ancestor_of=set()) + higher.get_latest_commit.side_effect = CommandError('branch gone') + + job = _make_job( + src_name='feature/BERTE-612-something', + dst_name='development/4.3', + dst_ancestors=set(), + cascade_branches=[dst, higher], + merge_base_map={}, + ) + check_source_branch_lineage(job) # must not raise + + def test_src_get_latest_commit_raises_skips_entire_check(self): + """If src.get_latest_commit() raises CommandError, skip the check.""" + dst = _make_branch('development/4.3', 'dst-tip', ancestor_of=set()) + higher = _make_branch('development/4', 'higher-tip', ancestor_of=set()) + + job = _make_job( + src_name='feature/BERTE-612-something', + dst_name='development/4.3', + dst_ancestors=set(), + cascade_branches=[dst, higher], + merge_base_map={ + ('src-tip', 'higher-tip'): 'foreign', + }, + ) + # Make src.get_latest_commit() fail + job.git.src_branch.get_latest_commit.side_effect = CommandError( + 'src gone' + ) + check_source_branch_lineage(job) # must not raise even though foreign + + def test_lower_branch_already_in_dst_is_skipped(self): + """Branches whose tip is already in dst are skipped without merge-base. + + This covers the dst.includes_commit(higher.get_latest_commit()) guard: + a branch whose full history is already absorbed into dst is skipped + since any merge-base with src would trivially be in dst too. + """ + dst_ancestors = {'lower-tip'} + dst = _make_branch('development/4.3', 'dst-tip', + ancestor_of=dst_ancestors) + lower = _make_branch('development/4.2', 'lower-tip', + ancestor_of=set()) + + job = _make_job( + src_name='feature/BERTE-612-something', + dst_name='development/4.3', + dst_ancestors=dst_ancestors, + cascade_branches=[dst, lower], + merge_base_map={}, # no merge-base call expected + ) + check_source_branch_lineage(job) # must not raise + + +class TestCheckSourceBranchLineageBackport: + """Source branch was already merged into the higher branch (legitimate + backport) — ForeignCommitsInSourceBranch must NOT be raised.""" + + def test_src_already_merged_into_higher(self): + """When higher already includes src tip, skip the contamination check. + + This models: PR was merged into dev/5.1 first, now being backported + to dev/4.3. higher.includes_commit(src.get_latest_commit()) is True. + + The merge_base_map is set so that without the backport guard the + merge-base would be 'foreign-commit', which is NOT in dst_ancestors, + making ForeignCommitsInSourceBranch fire. The guard must prevent that. + """ + dst = _make_branch('development/4.3', 'dst-tip', ancestor_of=set()) + # higher already includes 'src-tip' SHA (the default from _make_job) + higher = _make_branch('development/5.1', 'higher-tip', + ancestor_of={'src-tip'}) + + job = _make_job( + src_name='bugfix/TEST-0001', + dst_name='development/4.3', + dst_ancestors=set(), + cascade_branches=[dst, higher], + merge_base_map={ + ('src-tip', 'higher-tip'): 'foreign-commit', + }, + ) + check_source_branch_lineage(job) # must not raise + + def test_src_tip_equals_higher_tip_is_contamination(self): + """When src_sha == higher_tip the backport guard must NOT fire. + + A branch created directly from development/5.1 without any new commits + has src_sha == higher_tip. git considers every commit an ancestor of + itself, so higher.includes_commit(src_sha) would be True — incorrectly + treating this as a legitimate backport. The `src_sha != higher_tip` + pre-condition prevents this: the merge-base check is reached and + correctly detects the contamination. + """ + dst = _make_branch('development/4.3', 'dst-tip', ancestor_of=set()) + # higher_tip == 'src-tip': developer branched directly from dev/5.1 + higher = _make_branch('development/5.1', 'src-tip', + ancestor_of={'src-tip'}) + + job = _make_job( + src_name='feature/DIRECT-BRANCH-FROM-5.1', + dst_name='development/4.3', + dst_ancestors=set(), + cascade_branches=[dst, higher], + # merge-base(src, src) = src; not in dst → contamination + merge_base_map={ + ('src-tip', 'src-tip'): 'src-tip', + }, + ) + with pytest.raises(messages.ForeignCommitsInSourceBranch): + check_source_branch_lineage(job) + + +class TestCheckSourceBranchLineageContaminated: + """Source branch carries foreign history — ForeignCommitsInSourceBranch + must be raised.""" + + def test_raises_when_merge_base_not_in_dst(self): + """Merge-base with higher branch is NOT in dst → contamination.""" + dst_ancestors = {'dst-only-commit'} + dst = _make_branch('development/4.3', 'dst-tip', + ancestor_of=dst_ancestors) + higher = _make_branch('development/4', 'higher-tip', ancestor_of=set()) + + job = _make_job( + src_name='feature/ARTESCA-17922-fix', + dst_name='development/4.3', + dst_ancestors=dst_ancestors, + cascade_branches=[dst, higher], + merge_base_map={ + ('src-tip', 'higher-tip'): 'a5c998726' + }, + ) + with pytest.raises(messages.ForeignCommitsInSourceBranch): + check_source_branch_lineage(job) + + def test_error_contains_branch_names(self): + """Exception kwargs carry branch names and foreign_branches.""" + dst = _make_branch('development/4.3', 'dst-tip', ancestor_of=set()) + higher = _make_branch('development/4', 'higher-tip', ancestor_of=set()) + + job = _make_job( + src_name='feature/ARTESCA-17922-fix', + dst_name='development/4.3', + dst_ancestors=set(), + cascade_branches=[dst, higher], + merge_base_map={ + ('src-tip', 'higher-tip'): 'a5c998726' + }, + ) + with pytest.raises(messages.ForeignCommitsInSourceBranch) as exc_info: + check_source_branch_lineage(job) + + kwargs = exc_info.value.kwargs + assert kwargs['src_branch'] == 'feature/ARTESCA-17922-fix' + assert kwargs['dst_branch'] == 'development/4.3' + assert kwargs['foreign_branches'] == ['development/4'] + + def test_two_higher_branches_one_contaminated(self): + """Only the contaminated branch appears in foreign_branches.""" + dst_ancestors = {'base'} + dst = _make_branch('development/4.3', 'dst-tip', + ancestor_of=dst_ancestors) + higher1 = _make_branch('development/5.1', 'h1-tip', ancestor_of=set()) + higher2 = _make_branch('development/10.0', 'h2-tip', ancestor_of=set()) + + job = _make_job( + src_name='feature/ARTESCA-17922-fix', + dst_name='development/4.3', + dst_ancestors=dst_ancestors, + cascade_branches=[dst, higher1, higher2], + merge_base_map={ + ('src-tip', 'h1-tip'): 'foreign', + ('src-tip', 'h2-tip'): 'base', # clean + }, + ) + with pytest.raises(messages.ForeignCommitsInSourceBranch) as exc_info: + check_source_branch_lineage(job) + + assert exc_info.value.kwargs['foreign_branches'] == ['development/5.1'] + + def test_two_higher_branches_first_clean_second_contaminated(self): + """Loop skips clean higher branch and includes contaminated one.""" + dst_ancestors = {'base'} + dst = _make_branch('development/4.3', 'dst-tip', + ancestor_of=dst_ancestors) + higher1 = _make_branch('development/5.1', 'h1-tip', ancestor_of=set()) + higher2 = _make_branch('development/10.0', 'h2-tip', ancestor_of=set()) + + job = _make_job( + src_name='feature/ARTESCA-17922-fix', + dst_name='development/4.3', + dst_ancestors=dst_ancestors, + cascade_branches=[dst, higher1, higher2], + merge_base_map={ + ('src-tip', 'h1-tip'): 'base', # clean + ('src-tip', 'h2-tip'): 'foreign', # contaminated + }, + ) + with pytest.raises(messages.ForeignCommitsInSourceBranch) as exc_info: + check_source_branch_lineage(job) + + kwargs = exc_info.value.kwargs + assert kwargs['foreign_branches'] == ['development/10.0'] + + def test_two_higher_branches_both_contaminated(self): + """Both branches contaminated: both appear in foreign_branches.""" + dst_ancestors = set() + dst = _make_branch('development/4.3', 'dst-tip', + ancestor_of=dst_ancestors) + higher1 = _make_branch('development/5.1', 'h1-tip', ancestor_of=set()) + higher2 = _make_branch('development/10.0', 'h2-tip', ancestor_of=set()) + + job = _make_job( + src_name='feature/ARTESCA-17922-fix', + dst_name='development/4.3', + dst_ancestors=dst_ancestors, + cascade_branches=[dst, higher1, higher2], + merge_base_map={ + ('src-tip', 'h1-tip'): 'foreign-1', + ('src-tip', 'h2-tip'): 'foreign-2', + }, + ) + with pytest.raises(messages.ForeignCommitsInSourceBranch) as exc_info: + check_source_branch_lineage(job) + + assert exc_info.value.kwargs['foreign_branches'] == [ + 'development/5.1', 'development/10.0', + ] + + +class TestCheckSourceBranchLineageMultipleMergeBases: + """git merge-base --all can return multiple SHAs for criss-cross merges.""" + + def test_one_base_in_dst_one_not_is_contamination(self): + """If any merge-base is outside dst, the branch is contaminated. + + Simulates a criss-cross merge: merge-base --all returns two SHAs. + One is already in dst (would be a false negative without --all), + the other is not. The check must flag contamination. + """ + dst_ancestors = {'base-in-dst'} + dst = _make_branch('development/4.3', 'dst-tip', + ancestor_of=dst_ancestors) + higher = _make_branch('development/4', 'higher-tip', ancestor_of=set()) + + job = _make_job( + src_name='feature/ARTESCA-17922-fix', + dst_name='development/4.3', + dst_ancestors=dst_ancestors, + cascade_branches=[dst, higher], + # Simulate --all returning two bases: one clean, one foreign. + merge_base_map={ + ('src-tip', 'higher-tip'): 'base-in-dst\nforeign-base', + }, + ) + with pytest.raises(messages.ForeignCommitsInSourceBranch): + check_source_branch_lineage(job) + + def test_all_bases_in_dst_is_clean(self): + """If every merge-base is in dst, the branch is clean.""" + dst_ancestors = {'base1', 'base2'} + dst = _make_branch('development/4.3', 'dst-tip', + ancestor_of=dst_ancestors) + higher = _make_branch('development/4', 'higher-tip', ancestor_of=set()) + + job = _make_job( + src_name='feature/ARTESCA-17922-fix', + dst_name='development/4.3', + dst_ancestors=dst_ancestors, + cascade_branches=[dst, higher], + merge_base_map={ + ('src-tip', 'higher-tip'): 'base1\nbase2', + }, + ) + check_source_branch_lineage(job) # must not raise + + +class TestCheckSourceBranchLineageKnownLimitations: + """Document known false-positive scenarios (see function docstring).""" + + def test_extended_backport_false_positive(self): + """Known false positive: branch merged into higher, then extended. + + If a feature branch was previously merged into development/5.1 (so its + OLD tip is now in dev/5.1 history), and the developer adds new commits + before opening a backport PR targeting development/4.3, the backport + guard does NOT fire (new tip is not in dev/5.1). The merge-base with + dev/5.1 resolves to the developer's old tip, which is not in dev/4.3 + (it was merged to dev/5.1 only) — ForeignCommitsInSourceBranch is + raised even though the branch is clean. + + This test pins the behavior so it is visible if the heuristic changes. + """ + # 'old-tip' is the developer's previous tip that was merged into + # dev/5.1 but never cascaded to dev/4.3. + dst_ancestors = set() # 'old-tip' not in dev/4.3 + dst = _make_branch('development/4.3', 'dst-tip', + ancestor_of=dst_ancestors) + # higher includes 'old-tip' (was merged there) but NOT 'src-tip' + higher = _make_branch('development/5.1', 'h-tip', + ancestor_of={'old-tip'}) + + job = _make_job( + src_name='feature/BERTE-001-backport', + dst_name='development/4.3', + dst_ancestors=dst_ancestors, + cascade_branches=[dst, higher], + merge_base_map={ + # merge-base is 'old-tip': dev's own past commit, not a + # foreign commit from dev/5.1, but indistinguishable here. + ('src-tip', 'h-tip'): 'old-tip', + }, + ) + # Known false positive: ForeignCommitsInSourceBranch is raised even + # though the branch is clean. This test documents the limitation. + with pytest.raises(messages.ForeignCommitsInSourceBranch): + check_source_branch_lineage(job) + + +class TestBypassSourceBranchLineage: + """bypass_source_branch_lineage skips the check entirely.""" + + def _contaminated_job(self, bypass): + """Return a job that raises ForeignCommitsInSourceBranch.""" + dst_ancestors = set() + dst = _make_branch('development/4.3', 'dst-tip', + ancestor_of=dst_ancestors) + higher = _make_branch('development/4', 'h-tip', ancestor_of=set()) + return _make_job( + src_name='bugfix/BERTE-001', + dst_name='development/4.3', + dst_ancestors=dst_ancestors, + cascade_branches=[dst, higher], + merge_base_map={ + ('src-tip', 'h-tip'): 'foreign-sha', + }, + bypass=bypass, + ) + + def test_bypass_via_settings(self): + """bypass_source_branch_lineage=True on settings skips the check.""" + job = self._contaminated_job(bypass=True) + check_source_branch_lineage(job) # must not raise + + def test_bypass_via_author_bypass(self): + """bypass_source_branch_lineage in author_bypass skips the check.""" + job = self._contaminated_job(bypass=False) + job.author_bypass['bypass_source_branch_lineage'] = True + check_source_branch_lineage(job) # must not raise + + def test_no_bypass_still_raises(self): + """Without bypass, contamination is still detected.""" + job = self._contaminated_job(bypass=False) + with pytest.raises(messages.ForeignCommitsInSourceBranch): + check_source_branch_lineage(job) diff --git a/bert_e/workflow/gitwaterflow/__init__.py b/bert_e/workflow/gitwaterflow/__init__.py index 2fd750a0..3ad5c435 100644 --- a/bert_e/workflow/gitwaterflow/__init__.py +++ b/bert_e/workflow/gitwaterflow/__init__.py @@ -22,6 +22,7 @@ from bert_e import exceptions as messages from bert_e.job import handler, CommitJob, PullRequestJob, QueuesJob from bert_e.lib.cli import confirm +from bert_e.lib.simplecmd import CommandError from bert_e.reactor import Reactor, NotFound, NotPrivileged, NotAuthored from ..git_utils import push, clone_git_repo from ..pr_utils import find_comment, notify_user @@ -31,7 +32,8 @@ ) from .utils import ( bypass_incompatible_branch, bypass_peer_approval, - bypass_author_approval, bypass_leader_approval, bypass_build_status + bypass_author_approval, bypass_leader_approval, bypass_build_status, + bypass_source_branch_lineage ) from .commands import setup # noqa from .integration import (check_integration_branches, @@ -158,6 +160,7 @@ def _handle_pull_request(job: PullRequestJob): job.git.cascade.validate() check_branch_compatibility(job) + check_source_branch_lineage(job) jira_checks(job) check_integration_branches(job) @@ -383,6 +386,125 @@ def check_commit_diff(job): ) +def check_source_branch_lineage(job): + """Detect cross-branch contamination before any merge occurs. + + Raises ForeignCommitsInSourceBranch when the source branch shares history + with a release line that is higher than the target branch. This catches the + case where a developer rebased their feature branch on a Bert-E integration + commit (e.g. w/4) instead of directly on the target branch (e.g. + development/4.3), which would cause git to silently fast-forward the target + branch into the higher release line. + + Algorithm: for each development branch in the cascade that is NOT an + ancestor of dst, compute merge-base(src, higher). If that commit is not + an ancestor of dst, then src carries commits from the higher line that dst + does not know about. + + Note: for hotfix PRs, cascade.dst_branches contains only the single + matching hotfix branch, so this check is currently a no-op for hotfix + targets. + + Known limitation: if a feature branch was previously merged into a higher + release line and then extended with new commits before being backported, + the backport guard (which only tests the current tip) will not fire and + the merge-base check may produce a false positive because the shared + ancestor is the developer's own commit. In practice this scenario is rare + in GitWaterFlow, which cascades merges upward from the lowest target. + + Raises: + ForeignCommitsInSourceBranch + """ + if bypass_source_branch_lineage(job): + LOG.info('bypass_source_branch_lineage active, skipping lineage check') + return + + dst = job.git.dst_branch + src = job.git.src_branch + # Hoist to avoid O(N) subprocess calls and ensure the backport guard + # uses a consistent snapshot across all loop iterations. + try: + src_sha = src.get_latest_commit() + except CommandError: + LOG.debug('get_latest_commit(%s) failed, skipping lineage check', + src.name, exc_info=True) + return + + foreign_branches = [] + has_cascade_higher = False # any branch beyond dst exists in the cascade + had_higher = False # at least one higher branch was successfully resolved + for higher in job.git.cascade.dst_branches: + if higher.name == dst.name: + continue + has_cascade_higher = True + # Skip branches whose full history is already reachable from dst + # (e.g. a higher line that was previously cascaded into dst). + # If dst contains higher's tip, any merge-base(src, higher) is also + # guaranteed to be in dst, so the contamination check would be a no-op. + try: + higher_tip = higher.get_latest_commit() + except CommandError: + LOG.debug('get_latest_commit(%s) failed, skipping', + higher.name, exc_info=True) + continue + # had_higher is set only after a successful resolution so that an + # all-branches failure produces the correct diagnostic message. + had_higher = True + if dst.includes_commit(higher_tip): + continue + # src_sha == higher_tip means the branch is entirely on the higher + # release line — contamination (do NOT fire the backport guard; git + # considers every commit an ancestor of itself so the guard would + # incorrectly pass). Otherwise, src is a strict ancestor of higher: + # feature was previously merged there (legitimate backport), skip. + if src_sha != higher_tip and higher.includes_commit(src_sha): + continue + + try: + # Use pre-resolved SHAs (src_sha, higher_tip) for consistency with + # the snapshot already used by the backport guard above. + # --all covers criss-cross merges where two incomparable common + # ancestors exist; without it git picks one arbitrarily, and might + # return the ancestor already in dst, producing a false negative. + merge_bases = job.git.repo.cmd( + 'git merge-base --all %s %s', src_sha, higher_tip + ).split() + except CommandError: + LOG.debug('merge-base(%s, %s) failed, skipping', + src.name, higher.name, exc_info=True) + continue + + # merge-base --all yields ≥ 1 SHA on exit 0, but guard defensively. + if not merge_bases: + continue + + # Contamination: any merge-base that is not in dst means src carries + # history from the higher release line that dst does not know about. + # Optimisation: base == higher_tip implies dst.includes_commit was + # already evaluated as False above — skip the subprocess for that base. + if any(base == higher_tip or not dst.includes_commit(base) + for base in merge_bases): + foreign_branches.append(higher.name) + + if not foreign_branches: + if not has_cascade_higher: + LOG.debug('check_source_branch_lineage: no higher branches in ' + 'cascade for %s (hotfix target?), check is a no-op', + dst.name) + elif not had_higher: + LOG.debug('check_source_branch_lineage: all higher branches ' + 'failed to resolve for %s, check was not performed', + dst.name) + return + + raise messages.ForeignCommitsInSourceBranch( + src_branch=src.name, + dst_branch=dst.name, + foreign_branches=foreign_branches, + active_options=job.active_options, + ) + + def check_branch_compatibility(job): """Check that the pull request's source and destination branches are compatible with one another. diff --git a/bert_e/workflow/gitwaterflow/commands.py b/bert_e/workflow/gitwaterflow/commands.py index c492cf01..a3cbf26a 100644 --- a/bert_e/workflow/gitwaterflow/commands.py +++ b/bert_e/workflow/gitwaterflow/commands.py @@ -395,6 +395,11 @@ def setup(defaults={}): "Bypass the pull request leaders' approval", privileged=True, default=defaults.get("bypass_leader_approval", False)) + Reactor.add_option( + "bypass_source_branch_lineage", + "Bypass the cross-branch contamination check", + privileged=True, + default=defaults.get("bypass_source_branch_lineage", False)) # Other options Reactor.add_option( diff --git a/bert_e/workflow/gitwaterflow/utils.py b/bert_e/workflow/gitwaterflow/utils.py index bc710066..d3f03426 100644 --- a/bert_e/workflow/gitwaterflow/utils.py +++ b/bert_e/workflow/gitwaterflow/utils.py @@ -27,3 +27,8 @@ def bypass_build_status(job): def bypass_jira_check(job): return (job.settings.bypass_jira_check or job.author_bypass.get('bypass_jira_check', False)) + + +def bypass_source_branch_lineage(job): + return (job.settings.bypass_source_branch_lineage or + job.author_bypass.get('bypass_source_branch_lineage', False)) diff --git a/openwiki/architecture/gitwaterflow.md b/openwiki/architecture/gitwaterflow.md index 53f72dab..2ce92a28 100644 --- a/openwiki/architecture/gitwaterflow.md +++ b/openwiki/architecture/gitwaterflow.md @@ -63,24 +63,30 @@ that run. In order: 5. `clone_git_repo` — from here on the code operates on a real local clone. 6. `check_branch_compatibility` — source-branch prefix valid for destination (`bypass_incompatible_branch`). -7. `jira_checks` (`jira.py`) — ticket reference, project, issue type vs +7. `check_source_branch_lineage` — detects cross-branch contamination: raises + `ForeignCommitsInSourceBranch` (error 137, `status=failure`) when the + source branch shares history (via `git merge-base --all`) with a release + line higher than the target — e.g. a feature branch accidentally rebased on + a `w/` integration branch instead of `development/4.3` directly + (`bypass_source_branch_lineage`). +8. `jira_checks` (`jira.py`) — ticket reference, project, issue type vs branch prefix, Fix Version coherence (`bypass_jira_check`); includes the pre-GA hotfix "pending fix version" one-time reminder. -8. `check_commit_diff` — diff size limit (`max_commit_diff` setting). -9. `create_integration_branches` / `create_integration_pull_requests` +9. `check_commit_diff` — diff size limit (`max_commit_diff` setting). +10. `create_integration_branches` / `create_integration_pull_requests` (`integration.py`) — build the `w//` branches (and optionally PRs) for every destination in the cascade; `check_integration_branches` detects manual tampering and offers `reset`/`force_reset`. -10. `check_in_sync` / `check_pull_request_skew` — integration branches still +11. `check_in_sync` / `check_pull_request_skew` — integration branches still match the current PR head and destination. -11. `check_approvals` — author approval (not on GitHub) and peer/leader +12. `check_approvals` — author approval (not on GitHub) and peer/leader approval counts, with `unanimity`, `bypass_*_approval` options. -12. `check_build_status` / `revalidate_build_status` — build status on +13. `check_build_status` / `revalidate_build_status` — build status on integration branches must be green; the latter does a **live** re-check right before merging to avoid racing a stale cached status (`bypass_build_status`). -13. `merge_integration_branches` — the actual merge. If `use_queue` is +14. `merge_integration_branches` — the actual merge. If `use_queue` is enabled (default), this **adds PRs to the merge queue** instead of merging directly to `development/*` (see below); otherwise it merges each integration branch straight onto its destination.