From 47a3945fc1173a142978a0d70a62099afa68d06f Mon Sep 17 00:00:00 2001 From: Jonathan Borduas Date: Sun, 6 Sep 2026 00:59:23 +0100 Subject: [PATCH] =?UTF-8?q?--shape-only:=20skip=20leg=205=20too=20?= =?UTF-8?q?=E2=80=94=20three=20merge-time=20legs,=20fixed=20one=20at=20a?= =?UTF-8?q?=20time?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Measured on PR #599's own CI run: ⛔ 5 age at merge 9s since creation -> the advisory job went red A workflow triggered BY a PR's creation observes an age of seconds BY CONSTRUCTION, and it is not merging, so "was this old enough at merge?" is not a question it can ask. ⛔ THIS IS THE THIRD TIME, AND THAT IS THE FINDING. merge-guard has three legs that are facts about THE MERGE EVENT rather than about the PR: 0 holder == session a runner has no holder session fixed 1st 2 required gate it is INSIDE the run it asks about fixed 2nd 5 age at merge there is no merge yet fixed HERE ⚠ I fixed 0, then 2, and each time treated it as a one-off. I never re-read the remaining legs asking "which others have this property?" — so the same defect shipped three times in one tool, and each red cost a CI round trip. One instance is a bug; three is a class, and the class was visible after the first. ⇒ `--shape-only` now evaluates ONLY the shape legs — 1 base, 3 reviews, 4 three-dot diff — and NAMES each omission rather than passing it silently. ★ The control is a PAIR on identical input, because a skip with no known-negative is a bypass: seconds-old PR + --shape-only -> exit 0, "a merge that has not happened" seconds-old PR, no --shape-only -> exit 1, leg 5 BLOCKS #224 measured 25 of 100 PRs merged inside 60s of creation. That guard is the reason leg 5 exists, and the second test asserts the exemption did not weaken it. Suite 26/26 (was 24). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DTMf4EvaTEnDKXY47efRZZ --- tools/merge-guard.py | 27 ++++++++++++++++++++------- tools/test_merge_guard.py | 22 ++++++++++++++++++++++ 2 files changed, 42 insertions(+), 7 deletions(-) diff --git a/tools/merge-guard.py b/tools/merge-guard.py index ebabc66..b71b164 100644 --- a/tools/merge-guard.py +++ b/tools/merge-guard.py @@ -207,13 +207,26 @@ def leg(name, ok, detail): detail += f" ⛔ net-negative in {len(losers)} file(s): {top}{more}" leg("4 three-dot diff", add >= dele, detail) - created = d.get("createdAt") or "" - try: - age = (datetime.now(timezone.utc) - - datetime.fromisoformat(created.replace("Z", "+00:00"))).total_seconds() - leg("5 age at merge", age >= 120, f"{int(age)}s since creation") - except Exception: - leg("5 age at merge", False, f"UNESTABLISHED — unparseable createdAt {created!r}") + # ⛔ --shape-only OMITS LEG 5 TOO — the THIRD merge-time leg in this tool, and the one I + # left behind after fixing the other two. Measured on PR #599: + # ⛔ 5 age at merge 9s since creation -> the advisory job went red + # A workflow triggered BY the PR's creation observes an age of seconds BY CONSTRUCTION, + # and it is not merging, so "was this old enough at merge?" is not a question it can ask. + # ⇒ Legs 0, 2 and 5 are facts about THE MERGE EVENT; only 1, 3 and 4 are facts about the + # PR's SHAPE. ⚠ The name was right from the start and the implementation kept not catching + # up: I fixed leg 0, then leg 2, and each time treated it as a one-off instead of reading + # the remaining legs for the same property. Three instances is a class, not a coincidence. + if shape_only: + leg("5 age at merge", True, "⚠ SKIPPED — --shape-only. A run triggered BY this PR " + "cannot judge its age at a merge that has not happened.") + else: + created = d.get("createdAt") or "" + try: + age = (datetime.now(timezone.utc) + - datetime.fromisoformat(created.replace("Z", "+00:00"))).total_seconds() + leg("5 age at merge", age >= 120, f"{int(age)}s since creation") + except Exception: + leg("5 age at merge", False, f"UNESTABLISHED — unparseable createdAt {created!r}") return legs diff --git a/tools/test_merge_guard.py b/tools/test_merge_guard.py index f662d39..7af042c 100644 --- a/tools/test_merge_guard.py +++ b/tools/test_merge_guard.py @@ -15,6 +15,7 @@ import importlib.util import io import sys +from datetime import datetime, timezone import unittest from contextlib import redirect_stderr, redirect_stdout from pathlib import Path @@ -32,6 +33,9 @@ def load(): return mod +NOW_ISO = datetime.now(timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ") + + def prd(base="main", gate="SUCCESS", reviews=None, state="OPEN", created="2020-01-01T00:00:00Z", sha="deadbee"): return { @@ -222,6 +226,24 @@ def test_shape_only_SKIPS_leg0_and_says_so(self): self.assertIn("SKIPPED", out) self.assertIn("establishes\nNOTHING about who may merge".replace("\n", " "), out) + def test_shape_only_SKIPS_leg5_because_a_NEW_pr_is_seconds_old(self): + """⛔ THE KNOWN-POSITIVE, measured on PR #599: `5 age at merge 9s since creation`. + + A workflow triggered by the PR's own creation sees an age of seconds by construction. + It is not merging, so it cannot ask whether the PR was old enough AT MERGE. This is the + third merge-time leg in this tool (0 holder, 2 gate, 5 age) and the last to be fixed.""" + rc, out, _ = drive(self.mod, prd(created=NOW_ISO), ["1", "--shape-only"], session=OTHER) + self.assertEqual(rc, 0, "a seconds-old PR must not fail an ADVISORY shape check") + self.assertIn("a merge that has not happened", out) + + def test_the_SAME_new_pr_is_still_BLOCKED_from_a_real_merge(self): + """★ THE KNOWN-NEGATIVE. Identical input, --shape-only removed. #224 measured 25 of 100 + PRs merged inside 60s of creation; that guard must survive this exemption intact.""" + rc, out, _ = drive(self.mod, prd(created=NOW_ISO), ["1"]) + self.assertEqual(rc, 1, "a seconds-old PR is still BLOCKED from merging") + self.assertIn("5 age at merge", out) + self.assertNotIn("a merge that has not happened", out) + def test_shape_only_does_NOT_need_the_authority_file(self): """⛔ Found by CodeRabbit reviewing this PR, confirmed by measurement first.