diff --git a/.github/workflows/tools.yml b/.github/workflows/tools.yml index 5dc3038..fc905ea 100644 --- a/.github/workflows/tools.yml +++ b/.github/workflows/tools.yml @@ -204,3 +204,45 @@ jobs: echo " or the marker moved. Both print this line; neither is a pass." fi echo "⛔ This job is continue-on-error. Its green is NOT evidence." + + # ⛔ ADVISORY, AND DELIBERATELY NOT `continue-on-error`. The sibling job above uses it, + # and for that job it is right — its failures mean "CI has no fleet". Here a finding + # means a PR would REMOVE content it did not add, and `continue-on-error` renders that + # as a warning annotation: a detector firing into a stream nobody classifies (#397). + # + # ⇒ So this job goes RED on a finding and is NOT in the required-contexts list, which + # is the whole of "advisory": visible, and blocking nothing. ⛔ Adding it to that list + # is branch protection and operator-only (docs/MERGE-AUTHORITY.md rule 2). Nothing here + # touches it. + # + # ⚠ What this CANNOT check: who may merge. A runner has no holder session and cannot + # have one — the holder is a running pane. `--shape-only` omits leg 0 and SAYS SO on + # every run; it establishes nothing about authority. That half stays merge-time. + pr-shape: + name: PR shape (advisory, NOT gating) + if: github.event_name == 'pull_request' + runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: read + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 # ⛔ three-dot needs history; a shallow clone reads as empty + - uses: actions/setup-python@v5 + with: + python-version: '3.12' + + # ⇒ #510 leg 1: "a check exists — CI STEP or merge-time — that refuses a PR whose + # diff against main removes content the PR did not add." merge-time landed first; + # this is the other half, so the #572 class is caught when the PR opens rather than + # when someone happens to run the guard by hand. + - name: revert-shape and PR hygiene + env: + GH_TOKEN: ${{ github.token }} + run: | + git fetch origin main --quiet || true + # ⚠ POSITIONAL. merge-guard takes `prs ...`; late-push takes `--pr N`. The + # first version of this step wrote `--pr` here and CI refused it with exit 2 + # — the flag-strictness working, on the author of the flag-strictness. + python3 tools/merge-guard.py --shape-only ${{ github.event.pull_request.number }} diff --git a/tools/merge-guard.py b/tools/merge-guard.py index fd67e90..ebabc66 100644 --- a/tools/merge-guard.py +++ b/tools/merge-guard.py @@ -120,14 +120,24 @@ def pr_json(n, fields): raise Unestablished(f"gh --json {fields} was not parseable: {exc}") -def evaluate(n, session, authority_text): +def evaluate(n, session, authority_text, shape_only=False): legs = [] def leg(name, ok, detail): legs.append((name, ok, detail)) - ok, detail = holder_check(authority_text, session) - leg("0 holder == session", ok, detail) + # ⛔ --shape-only OMITS leg 0 AND SAYS SO. It exists for CI, which has no holder + # session and cannot have one: the holder is a running pane, not a runner. A flag + # that made leg 0 PASS without a session would be a hole shaped exactly like the + # thing this tool guards, so it is omitted and named rather than defaulted true. + # ⇒ Shape-only ESTABLISHES NOTHING ABOUT AUTHORITY. It answers #510 leg 1's other + # half — "a check exists, CI step OR merge-time" — and only that half. + if shape_only: + leg("0 holder == session", True, "⚠ SKIPPED — --shape-only. This run establishes " + "NOTHING about who may merge.") + else: + ok, detail = holder_check(authority_text, session) + leg("0 holder == session", ok, detail) d = pr_json(n, "baseRefName,mergeStateStatus,reviews,headRefOid,createdAt,state,statusCheckRollup") if d.get("state") != "OPEN": @@ -135,13 +145,30 @@ def leg(name, ok, detail): leg("1 base == main", d["baseRefName"] == "main", d["baseRefName"]) - rollup = d.get("statusCheckRollup") or [] - req = [c for c in rollup if (c.get("name") or c.get("context") or "") == "hermetic suites (gating)"] - if not req: - leg("2 required gate", False, "UNESTABLISHED — 'hermetic suites (gating)' absent from rollup") + # ⛔ --shape-only OMITS LEG 2 TOO, and for a reason measured on this tool's own PR. + # A CI job asking "is the required gate green?" from INSIDE the run that CONTAINS + # that gate is asking a self-referential question. Measured on PR #597, both jobs + # in the same run: + # hermetic suites (gating) started 23:22:19 completed 23:23:29 success + # PR shape (advisory) started 23:22:19 completed 23:22:25 FAILURE + # ⇒ pr-shape read the gate SIX SECONDS in, 64s before the gate finished. The gate + # was necessarily unfinished, so leg 2 was necessarily unestablished, and the job + # went red for a fact about ITS OWN CONCURRENCY rather than about the PR. + # ⚠ `needs:` would serialise it, but that is the wrong fix: it makes an ADVISORY job + # a prerequisite of nothing while doubling the run's latency, and it still leaves the + # runner asserting a green gate that the merger must re-read at merge time anyway. + # ⇒ The honest move is the same one leg 0 already makes: SKIP AND SAY SO. + if shape_only: + leg("2 required gate", True, "⚠ SKIPPED — --shape-only. A run cannot establish " + "the outcome of a gate it CONTAINS. Re-read at merge.") else: - concl = req[0].get("conclusion") or req[0].get("state") or "" - leg("2 required gate", concl == "SUCCESS", concl or "UNESTABLISHED") + rollup = d.get("statusCheckRollup") or [] + req = [c for c in rollup if (c.get("name") or c.get("context") or "") == "hermetic suites (gating)"] + if not req: + leg("2 required gate", False, "UNESTABLISHED — 'hermetic suites (gating)' absent from rollup") + else: + concl = req[0].get("conclusion") or req[0].get("state") or "" + leg("2 required gate", concl == "SUCCESS", concl or "UNESTABLISHED") revs = d.get("reviews") or [] changes = [r for r in revs if r.get("state") == "CHANGES_REQUESTED"] @@ -236,6 +263,9 @@ def main(): ap.add_argument("--session", default=os.environ.get("CLAUDE_CODE_SESSION_ID", ""), help="session id to test as (default: $CLAUDE_CODE_SESSION_ID)") ap.add_argument("--authority", default=AUTHORITY, help=f"path to {AUTHORITY}") + ap.add_argument("--shape-only", action="store_true", + help="omit the holder leg — for CI, which has no holder session. " + "⛔ Establishes nothing about authority.") ap.add_argument("--self-test", action="store_true", help="run the controls; no network") args = ap.parse_args() @@ -248,6 +278,10 @@ def main(): # condition's own command could not be satisfied by the instrument written for it. # ⇒ A holder check needs no PR: "may this session merge at all?" is answerable, and # is exactly the question those four issues pose. + if not args.prs and args.shape_only: + print("⛔ VOID — --shape-only needs a PR: without one there is no shape to check, " + "and it is not a holder check.", file=sys.stderr) + return 2 if not args.prs: try: text = Path(args.authority).read_text(encoding="utf-8") @@ -260,19 +294,32 @@ def main(): if ok else "REFUSED — this session is not the holder")) return 0 if ok else 1 - try: - text = Path(args.authority).read_text(encoding="utf-8") - except OSError as exc: - print(f"⛔ VOID — cannot read {args.authority}: {exc}\n" - f" ADDABLE — run from a checkout that has it, or pass --authority.", - file=sys.stderr) - return 2 + # ⛔ --shape-only MUST NOT READ THE AUTHORITY FILE. It skips leg 0, so `text` is + # never used — but reading it made a MISSING file fatal, and CI checks out the PR's + # own tree. ⇒ A PR that MOVED OR DELETED docs/MERGE-AUTHORITY.md turned pr-shape red + # with exit 2 for a fact about the authority record, not about the PR's shape. Same + # defect as leg 2 read from inside its own run: the advisory job going red for the + # wrong reason, which is how an advisory job stops being read at all. + # ⇒ Found by CodeRabbit in review of this PR, and confirmed here by measurement + # before it was believed: + # --shape-only 597 --authority /nonexistent/AUTH.md exit 2 + # --shape-only 597 exit 0 + if args.shape_only: + text = "" + else: + try: + text = Path(args.authority).read_text(encoding="utf-8") + except OSError as exc: + print(f"⛔ VOID — cannot read {args.authority}: {exc}\n" + f" ADDABLE — run from a checkout that has it, or pass --authority.", + file=sys.stderr) + return 2 worst = 0 for arg in args.prs: print(f"══ PR #{arg} ══") try: - legs = evaluate(int(arg), args.session, text) + legs = evaluate(int(arg), args.session, text, args.shape_only) except (Unestablished, ValueError) as exc: print(f" ⛔ UNESTABLISHED — {exc}") print(" ⇒ BLOCK. A leg that cannot be measured is not a leg that passed.\n") diff --git a/tools/test_merge_guard.py b/tools/test_merge_guard.py index 702f1dc..f662d39 100644 --- a/tools/test_merge_guard.py +++ b/tools/test_merge_guard.py @@ -212,6 +212,99 @@ def test_no_pr_named_is_the_HOLDER_CHECK_not_a_refusal(self): sys.argv = old self.assertEqual(rc_void, 2, "an unreadable authority file is still VOID") + # ── --shape-only, for CI ── + + def test_shape_only_SKIPS_leg0_and_says_so(self): + """⛔ It must not read as authorization. A runner has no holder session and + cannot have one, so leg 0 is OMITTED and NAMED — never quietly passed.""" + rc, out, _ = drive(self.mod, prd(), ["1", "--shape-only"], session=OTHER) + self.assertEqual(rc, 0, "the shape legs pass; the holder leg is not evaluated") + self.assertIn("SKIPPED", out) + self.assertIn("establishes\nNOTHING about who may merge".replace("\n", " "), out) + + def test_shape_only_does_NOT_need_the_authority_file(self): + """⛔ Found by CodeRabbit reviewing this PR, confirmed by measurement first. + + --shape-only skips leg 0, so the authority text is never used — but main() read + the file anyway and returned 2 when it was missing. CI checks out the PR's OWN + tree, so a PR that MOVED docs/MERGE-AUTHORITY.md turned pr-shape red for a fact + about the authority record rather than about the PR's shape. + + ⇒ The shape legs must still be evaluated with no authority file at all.""" + self.mod.pr_json = lambda n, f: prd() + self.mod.sh = lambda a, allow_fail=False: ("1\t900\ttools/README.md" + if a[:2] == ["git", "diff"] else "") + old = sys.argv + sys.argv = ["merge-guard.py", "--shape-only", "1", + "--session", OTHER, "--authority", "/nonexistent/AUTH.md"] + out = io.StringIO() + try: + with redirect_stdout(out), redirect_stderr(io.StringIO()): + rc = self.mod.main() + finally: + sys.argv = old + self.assertNotEqual(rc, 2, "a missing authority file must not VOID a shape check") + # ★ and it must have REACHED the shape legs, not merely exited non-2 + self.assertIn("net-negative in 1 file(s)", out.getvalue()) + self.assertEqual(rc, 1, "it blocks on the REVERT, which is what it is for") + + def test_a_missing_authority_STILL_voids_a_real_merge(self): + """★ THE KNOWN-NEGATIVE. The exemption above is scoped to --shape-only ONLY. + Without it, an unreadable authority record still establishes nothing about who + may merge, and must still be VOID — otherwise the fix is a bypass.""" + old = sys.argv + sys.argv = ["merge-guard.py", "1", "--session", HOLDER, + "--authority", "/nonexistent/AUTH.md"] + try: + with redirect_stdout(io.StringIO()), redirect_stderr(io.StringIO()): + rc = self.mod.main() + finally: + sys.argv = old + self.assertEqual(rc, 2, "no authority record ⇒ VOID, for a real merge") + + def test_shape_only_SKIPS_leg2_because_the_gate_is_IN_ITS_OWN_RUN(self): + """⛔ THE KNOWN-POSITIVE, and it is this tool's own PR #597. + + A pr-shape job and the gating job start in the SAME run at the same instant. + pr-shape finished at 23:22:25; the gate finished at 23:23:29. So pr-shape read + a gate with `conclusion: null` and went red for a fact about concurrency, not + about the PR. Modelled here EXACTLY as GitHub reports it: an in-progress check + run carries conclusion null, not a falsy string.""" + d = prd() + d["statusCheckRollup"] = [{"name": "hermetic suites (gating)", + "conclusion": None, "status": "IN_PROGRESS"}] + rc, out, _ = drive(self.mod, d, ["1", "--shape-only"], session=OTHER) + self.assertEqual(rc, 0, "an unfinished gate must not fail an ADVISORY shape check") + self.assertIn("a gate it CONTAINS", out) + + def test_the_SAME_pending_gate_still_BLOCKS_a_real_merge(self): + """★ THE KNOWN-NEGATIVE, without which the fix above is just a hole. + + Identical input, `--shape-only` removed. The skip must be scoped to the advisory + path ONLY: at merge time a pending gate is not a green one, and leg 2 must still + refuse. Same data, opposite verdict — that is what makes the pair evidence.""" + d = prd() + d["statusCheckRollup"] = [{"name": "hermetic suites (gating)", + "conclusion": None, "status": "IN_PROGRESS"}] + rc, out, _ = drive(self.mod, d, ["1"]) # HOLDER session, no --shape-only + self.assertEqual(rc, 1, "a pending gate BLOCKS a merge") + self.assertIn("2 required gate", out) + self.assertNotIn("a gate it CONTAINS", out) + + def test_shape_only_still_catches_a_revert(self): + """The whole point: #572's class caught at PR time, not merge time.""" + rc, out, _ = drive(self.mod, prd(), ["1", "--shape-only"], session=OTHER, + numstat="1\t900\ttools/README.md") + self.assertEqual(rc, 1) + self.assertIn("net-negative in 1 file(s)", out) + + def test_shape_only_without_a_pr_is_VOID_not_a_holder_check(self): + """⛔ The bypass that must not exist: --shape-only with no PR could otherwise + read as 'this session may merge'. It refuses.""" + rc, _, err = drive(self.mod, prd(), ["--shape-only"], session=OTHER) + self.assertEqual(rc, 2) + self.assertIn("not a holder check", err) + # ── ⛔ criterion 4: shown to FAIL ── def test_control_can_fail(self):