From 03611cf39de1bc276d6d47b17a227fd44dff3c01 Mon Sep 17 00:00:00 2001 From: Don Kendall Date: Mon, 13 Jul 2026 13:38:02 -0400 Subject: [PATCH] fix(ci): guard series-branch mirrors before merge-upstream, not after MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit merge-upstream cannot fast-forward a branch that is ahead of upstream, so it silently mints a merge commit and returns 200 anyway. That leaves the branch ahead forever, so the next run mints another. One orphaned forward-port.yml commit (install_forward_port was flipped to false, but the distributor only ever adds, never removes) seeded ~140 merge commits across 14 OCA series branches. The post-hoc divergence check could not catch it: MANAGED_COMMIT_PATTERNS whitelisted /^Merge branch '...' into /, so every merge commit the tool produced was classified as an expected structural overlay. The mechanism laundered its own damage and the digest reported the branches as healthy. - Run the compare BEFORE merge-upstream and refuse to merge a mirror branch that is ahead. Reporting after the fact only documents the damage; checking first prevents it. - Drop the merge-commit whitelist. A merge commit on a series branch is always a bug, so it must always be reportable. - Non-OCA forks (upstream_org: null) are unaffected — sentry/master legitimately carries customizations, and merging upstream into it is the correct primitive. The 14 affected branches were reset to their upstream tips separately; nothing was lost (all were behind_by=0, and the only content delta was the dead forward-port.yml). Claude-Session: https://claude.ai/code/session_014td9mfVrkca9eeCR7SMpAb --- .github/scripts/fork_sync_digest.py | 101 ++++++---- .github/scripts/render_digest.py | 24 +-- .../scripts/tests/test_sync_branch_state.py | 185 ++++++++++-------- 3 files changed, 174 insertions(+), 136 deletions(-) diff --git a/.github/scripts/fork_sync_digest.py b/.github/scripts/fork_sync_digest.py index fca8ea3..4d14d37 100644 --- a/.github/scripts/fork_sync_digest.py +++ b/.github/scripts/fork_sync_digest.py @@ -39,24 +39,23 @@ # the token env). Populated by main() before any gh() call. HEADERS: dict = {} -# Ahead-commit subjects that the sync pipeline itself produces, so an -# overlay of these on top of upstream is expected — not "diverged" in -# the alarming sense. Two sources: -# 1. distribute_forward_port.py writes forward-port.yml via the -# Contents API, which lands as a commit on the default (series) -# branch. The workflow file MUST live on the branch where -# pull_request_target events fire, so this commit is structural. -# 2. merge-upstream produces a "Merge branch 'OCA:' into " -# commit any time the fork is non-empty (i.e. carries #1). Also -# structural — not a sign of human error on a series branch. -# Anything outside these patterns is a real divergence (someone pushed -# work to a series branch directly) and stays flagged. +# Ahead-commit subjects the sync pipeline itself produces. Used only to +# word the digest entry ("this is the distributor's doing, not a stray +# human push") — it never suppresses the mirror guard below. +# +# A "Merge branch 'OCA:' into " pattern deliberately does NOT +# appear here. It used to, and that is precisely how ~140 merge commits +# accumulated unseen across 14 series branches: one orphaned +# forward-port.yml commit made fast-forward impossible, merge-upstream +# minted a merge commit on every run, and the whitelist classified each +# one as expected. The mechanism laundered its own damage. Series +# branches are exact upstream mirrors; a merge commit on one is always +# a bug, so it must always be reportable. MANAGED_COMMIT_PATTERNS = ( re.compile( r"^chore\(ci\): (created|updated) forward-port\.yml " r"from ledoent/\.github distributor" ), - re.compile(r"^Merge branch '[^']+' into "), ) @@ -121,42 +120,36 @@ def load_forks(path: str | Path = ".github/forks.yml") -> list[dict]: def sync_branch(repo: str, branch: str, upstream_org: str | None = None) -> dict: - status, body = gh("POST", f"/repos/{repo}/merge-upstream", {"branch": branch}) - msg = body.get("message", "") - # "Branch n/a" responses we expect on forks that haven't cut a 20.0 - # (or any future release) yet. Two API shapes for the same condition: - # 422 + "does not exist" — branch isn't on the fork at all - # 404 + "Branch not found" — branch isn't on the upstream either - # Both are "skipped", not "failed". - skipped = (status == 422 and "does not exist" in msg.lower()) or ( - status == 404 and "branch not found" in msg.lower() - ) result = { "repo": repo, "branch": branch, - "status": status, - "message": msg, - "merge_type": body.get("merge_type"), - "skipped": skipped, + "status": None, + "message": "", + "merge_type": None, + "skipped": False, "ahead_by": None, "behind_by": None, "ahead_commits": [], "diverged": False, "managed_overlay": False, } - # Post-sync divergence check. merge-upstream silently produces a - # merge commit (instead of fast-forwarding) when the fork has local - # commits the upstream doesn't have — returning success either way. - # Without this check, accidental pushes to a series-named branch on - # the fork accumulate undetected. + + # Mirror guard — runs BEFORE merge-upstream, and that order is the + # whole point. merge-upstream cannot fast-forward a branch that is + # ahead of upstream, so it silently mints a merge commit instead and + # still returns 200. That merge commit leaves the branch ahead + # forever, so the next run mints another. Checking afterwards only + # documents the damage; checking first prevents it. + # + # A fork series branch is by definition an exact copy of upstream. + # If it is ahead by even one commit it is no longer a mirror, and + # the fix is a human deciding what to do with those commits — not a + # merge. So: report, and leave the branch alone. # - # But: forks with install_forward_port carry a managed chore commit - # on every series branch (the distributor's forward-port.yml write), - # and merge-upstream then layers a "Merge branch 'OCA:'" commit - # on top each time upstream advances. Both are structural — not - # human error. Classify them as `managed_overlay` and reserve - # `diverged` for genuinely unexpected ahead-commits. - if upstream_org and not skipped and status < 400: + # (Only forks with an upstream_org are mirrors. Non-OCA forks — + # sentry, odoo-cloud-platform — legitimately carry customizations on + # the tracked branch, and merging upstream into them is correct.) + if upstream_org: ahead, behind, subjects = _compare_with_upstream( repo, upstream_org, branch ) @@ -165,10 +158,32 @@ def sync_branch(repo: str, branch: str, upstream_org: str | None = None) -> dict result["ahead_commits"] = subjects if ahead and ahead > 0: unmanaged = [s for s in subjects if not _is_managed_commit(s)] - if unmanaged: - result["diverged"] = True - else: - result["managed_overlay"] = True + result["diverged"] = bool(unmanaged) + result["managed_overlay"] = not unmanaged + result["status"] = 200 + result["message"] = ( + f"not a mirror: {ahead} commit(s) ahead of " + f"{upstream_org}:{branch} — refusing to merge (that would " + f"add a merge commit and entrench the drift). Reset the " + f"branch to the upstream tip, or move the commits to a " + f"feature branch." + ) + return result + + status, body = gh("POST", f"/repos/{repo}/merge-upstream", {"branch": branch}) + msg = body.get("message", "") + # "Branch n/a" responses we expect on forks that haven't cut a 20.0 + # (or any future release) yet. Two API shapes for the same condition: + # 422 + "does not exist" — branch isn't on the fork at all + # 404 + "Branch not found" — branch isn't on the upstream either + # Both are "skipped", not "failed". + skipped = (status == 422 and "does not exist" in msg.lower()) or ( + status == 404 and "branch not found" in msg.lower() + ) + result["status"] = status + result["message"] = msg + result["merge_type"] = body.get("merge_type") + result["skipped"] = skipped return result diff --git a/.github/scripts/render_digest.py b/.github/scripts/render_digest.py index a84b1a8..f559b0e 100644 --- a/.github/scripts/render_digest.py +++ b/.github/scripts/render_digest.py @@ -53,18 +53,18 @@ def render( if r.get("merge_type") == "none" and r["status"] < 400 ] sync_skipped = [r for r in sync_only if r["skipped"]] - # Divergence = fork branch has commits upstream doesn't, AND those - # commits aren't part of the managed overlay (forward-port.yml - # chore commit + sync-produced merge commits). Real divergence - # means someone pushed work to a series branch — needs manual fix. + # Divergence = a series branch has commits upstream doesn't, by any + # hand other than the distributor's. Someone pushed work to a mirror + # branch. The sync refuses to merge it (a merge would only entrench + # the drift), so it stays broken until a human resets it. sync_diverged = [ r for r in sync_only if r.get("diverged") and not r["skipped"] ] - # Managed-overlay branches are 1+ ahead but only by structural - # commits. Counted in a small footer for transparency, never in the - # subject line — they're the steady state for install_forward_port - # forks and would otherwise spam the inbox every day. + # Ahead only by the distributor's forward-port.yml chore commit. + # Still not a mirror, so still not merged — but attributable to the + # pipeline rather than a stray push, so it gets a footer line instead + # of the subject-line alarm. sync_managed_overlay = [ r for r in sync_results if r.get("managed_overlay") and not r["skipped"] @@ -249,9 +249,11 @@ def render( lines.append( '

' f"Managed overlay: {len(sync_managed_overlay)} branches " - "are 1+ ahead of upstream only by structural commits " - "(forward-port.yml distributor + sync-produced merges). " - "Expected; not divergence.

" + "are ahead of upstream by the forward-port.yml distributor " + "commit alone. Not a stray push — but not an exact mirror " + "either, so they were not merged. Reset them to the " + "upstream tip (or turn off install_forward_port) " + "to get them syncing again.

" ) lines.append( '

' diff --git a/.github/scripts/tests/test_sync_branch_state.py b/.github/scripts/tests/test_sync_branch_state.py index f31ce93..1e0d7c2 100644 --- a/.github/scripts/tests/test_sync_branch_state.py +++ b/.github/scripts/tests/test_sync_branch_state.py @@ -59,92 +59,123 @@ def test_conflict_409_is_not_skipped(monkeypatch): def _patch_gh_sequence(monkeypatch, *returns): - """Make gh() return each tuple in turn (merge-upstream, then compare).""" + """Make gh() return each tuple in turn. + + On a fork with an upstream_org the compare call comes FIRST (the + mirror guard), and merge-upstream only runs if the branch is not + ahead. Order the tuples accordingly. + """ it = iter(returns) monkeypatch.setattr(fork_sync_digest, "gh", lambda *a, **kw: next(it)) +def _record_gh(monkeypatch, *returns): + """Same, but also records (method, path) of every call made.""" + calls = [] + it = iter(returns) + + def fake_gh(method, path, body=None): + calls.append((method, path)) + return next(it) + + monkeypatch.setattr(fork_sync_digest, "gh", fake_gh) + return calls + + def _commits(*subjects): return [{"commit": {"message": s}} for s in subjects] +def _compare(ahead, *subjects, behind=0): + return (200, { + "ahead_by": ahead, + "behind_by": behind, + "commits": _commits(*subjects), + }) + + def test_diverged_branch_is_flagged_when_upstream_org_provided(monkeypatch): - # merge-upstream succeeds with a merge commit; compare reports 30 - # local commits ahead, one of which isn't a managed overlay. This - # is the case that motivated the check — ledoent/social:18.0 in - # May 2026 silently accumulated drift because merge-upstream - # returns success either way. - _patch_gh_sequence( + # A stray human commit on a series branch. Must be flagged AND must + # not be merged — merging would bury it under a merge commit. + calls = _record_gh( monkeypatch, - (200, {"merge_type": "merge"}), - (200, { - "ahead_by": 30, - "behind_by": 0, - "commits": _commits( - "chore(ci): created forward-port.yml from ledoent/.github distributor", - "Merge branch 'OCA:18.0' into 18.0", - "[FIX] some real human commit that shouldn't be here", - ), - }), + _compare(30, "[FIX] some real human commit that shouldn't be here"), ) r = fork_sync_digest.sync_branch("ledoent/social", "18.0", upstream_org="OCA") assert r["diverged"] is True assert r["managed_overlay"] is False assert r["ahead_by"] == 30 - assert r["behind_by"] == 0 + assert not any(m == "POST" for m, _ in calls), "must not merge a diverged branch" + + +def test_ahead_branch_is_never_merged(monkeypatch): + # THE regression this guard exists for. merge-upstream on a branch + # that is ahead cannot fast-forward, so it mints a merge commit and + # returns 200 — leaving the branch ahead forever, so the next run + # mints another. 14 series branches accumulated ~140 of these before + # anyone noticed. The guard must refuse to call merge-upstream at all. + calls = _record_gh(monkeypatch, _compare(1, "Merge branch 'OCA:18.0' into 18.0")) + r = fork_sync_digest.sync_branch("ledoent/web", "18.0", upstream_org="OCA") + assert calls == [("GET", "/repos/OCA/web/compare/OCA:18.0...ledoent:18.0")] + assert r["merge_type"] is None + assert "refusing to merge" in r["message"] + + +def test_merge_commit_is_no_longer_whitelisted(monkeypatch): + # The old MANAGED_COMMIT_PATTERNS matched "Merge branch '...' into", + # so the tool's own merge commits were classified as expected and the + # drift stayed invisible. A merge commit on a mirror branch is always + # a bug and must now surface as divergence. + _patch_gh_sequence(monkeypatch, _compare(2, "Merge branch 'OCA:18.0' into 18.0")) + r = fork_sync_digest.sync_branch("ledoent/web", "18.0", upstream_org="OCA") + assert r["diverged"] is True + assert r["managed_overlay"] is False def test_clean_fast_forward_is_not_diverged(monkeypatch): _patch_gh_sequence( monkeypatch, + _compare(0), (200, {"merge_type": "fast-forward"}), - (200, {"ahead_by": 0, "behind_by": 0, "commits": []}), ) r = fork_sync_digest.sync_branch("ledoent/x", "18.0", upstream_org="OCA") assert r["diverged"] is False assert r["managed_overlay"] is False + assert r["merge_type"] == "fast-forward" assert r["ahead_by"] == 0 -def test_only_distributor_chore_is_managed_not_diverged(monkeypatch): - # The steady state for install_forward_port forks: 1 ahead by the - # chore commit alone. Must NOT trip the diverged alarm. +def test_behind_only_branch_still_fast_forwards(monkeypatch): + # Stale but pristine (a strict ancestor of upstream) — the common + # case. Must still sync. _patch_gh_sequence( monkeypatch, - (200, {"merge_type": "none"}), - (200, { - "ahead_by": 1, - "behind_by": 0, - "commits": _commits( - "chore(ci): created forward-port.yml from ledoent/.github distributor", - ), - }), + _compare(0, behind=4), + (200, {"merge_type": "fast-forward"}), ) - r = fork_sync_digest.sync_branch("ledoent/web", "18.0", upstream_org="OCA") + r = fork_sync_digest.sync_branch("ledoent/OpenUpgrade", "19.0", upstream_org="OCA") + assert r["merge_type"] == "fast-forward" assert r["diverged"] is False - assert r["managed_overlay"] is True - assert r["ahead_by"] == 1 + assert r["behind_by"] == 4 -def test_chore_plus_sync_merge_is_managed_not_diverged(monkeypatch): - # After an upstream advance, the chore + a "Merge branch 'OCA:...'" - # land together. Still structural, still not divergence. - _patch_gh_sequence( +def test_only_distributor_chore_is_managed_not_diverged(monkeypatch): + # The distributor's forward-port.yml commit. Named as managed so the + # digest can word it accurately ("the distributor did this, not a + # human") — but it still leaves the branch a non-mirror, so it is + # still not merged. + calls = _record_gh( monkeypatch, - (200, {"merge_type": "merge"}), - (200, { - "ahead_by": 2, - "behind_by": 0, - "commits": _commits( - "chore(ci): created forward-port.yml from ledoent/.github distributor", - "Merge branch 'OCA:18.0' into 18.0", - ), - }), + _compare( + 1, + "chore(ci): created forward-port.yml from ledoent/.github distributor", + ), ) r = fork_sync_digest.sync_branch("ledoent/web", "18.0", upstream_org="OCA") assert r["diverged"] is False assert r["managed_overlay"] is True - assert r["ahead_by"] == 2 + assert r["ahead_by"] == 1 + assert not any(m == "POST" for m, _ in calls) def test_updated_chore_subject_is_also_managed(monkeypatch): @@ -153,14 +184,10 @@ def test_updated_chore_subject_is_also_managed(monkeypatch): # be recognised. _patch_gh_sequence( monkeypatch, - (200, {"merge_type": "none"}), - (200, { - "ahead_by": 1, - "behind_by": 0, - "commits": _commits( - "chore(ci): updated forward-port.yml from ledoent/.github distributor", - ), - }), + _compare( + 1, + "chore(ci): updated forward-port.yml from ledoent/.github distributor", + ), ) r = fork_sync_digest.sync_branch("ledoent/web", "18.0", upstream_org="OCA") assert r["managed_overlay"] is True @@ -168,29 +195,25 @@ def test_updated_chore_subject_is_also_managed(monkeypatch): def test_no_upstream_org_skips_divergence_check(monkeypatch): - # Non-OCA forks (upstream_org: null) get no compare call. Verified by - # patching gh to fail loudly if called more than once. - calls = [] - - def fake_gh(method, path, body=None): - calls.append((method, path)) - return 200, {"merge_type": "fast-forward"} - - monkeypatch.setattr(fork_sync_digest, "gh", fake_gh) - r = fork_sync_digest.sync_branch("ledoent/x", "master") - assert len(calls) == 1 + # Non-OCA forks (upstream_org: null) are NOT mirrors — sentry/master + # carries real customizations — so they get no guard and no compare + # call. merge-upstream is the correct primitive there. + calls = _record_gh(monkeypatch, (200, {"merge_type": "merge"})) + r = fork_sync_digest.sync_branch("ledoent/sentry", "master") + assert calls == [("POST", "/repos/ledoent/sentry/merge-upstream")] assert r["diverged"] is False assert r["ahead_by"] is None + assert r["merge_type"] == "merge" -def test_compare_failure_does_not_mask_successful_sync(monkeypatch): - # If the compare endpoint flakes, the sync result still records the - # merge-upstream success. ahead_by stays None so the digest knows we - # don't actually know — better than a false "clean" classification. +def test_compare_failure_does_not_block_the_sync(monkeypatch): + # If the compare endpoint flakes we cannot prove the branch is ahead. + # Fall through to merge-upstream rather than stall the sync, and + # leave ahead_by None so the digest knows we don't actually know. _patch_gh_sequence( monkeypatch, - (200, {"merge_type": "fast-forward"}), (500, {"message": "Internal Server Error"}), + (200, {"merge_type": "fast-forward"}), ) r = fork_sync_digest.sync_branch("ledoent/x", "18.0", upstream_org="OCA") assert r["status"] == 200 @@ -200,19 +223,17 @@ def test_compare_failure_does_not_mask_successful_sync(monkeypatch): assert r["ahead_by"] is None -def test_failed_sync_skips_compare(monkeypatch): - # No point burning a compare call if merge-upstream itself failed — - # the failure already triggers a digest alarm. - calls = [] - - def fake_gh(method, path, body=None): - calls.append((method, path)) - return 403, {"message": "Resource not accessible by personal access token"} - - monkeypatch.setattr(fork_sync_digest, "gh", fake_gh) +def test_failed_sync_is_still_reported(monkeypatch): + # The PAT-scope error class. Compare says clean, merge-upstream then + # fails — the failure must survive into the result. + _patch_gh_sequence( + monkeypatch, + _compare(0), + (403, {"message": "Resource not accessible by personal access token"}), + ) r = fork_sync_digest.sync_branch("ledoent/x", "18.0", upstream_org="OCA") - assert len(calls) == 1 assert r["status"] == 403 + assert r["skipped"] is False assert r["diverged"] is False