diff --git a/scripts/bugbot-gate.py b/scripts/bugbot-gate.py index eca40c9..2369b0d 100644 --- a/scripts/bugbot-gate.py +++ b/scripts/bugbot-gate.py @@ -316,6 +316,7 @@ comments(first: 1) { nodes { author { login } + originalCommit { oid } body url } @@ -388,6 +389,28 @@ def query_lacks_author_kind(query=QUERY): return re.search(r"author\s*\{[^}]*__typename", query) is None +def query_lacks_finding_commit(query=QUERY): + """True when `query` no longer asks which commit a finding was raised against. + + `findings` reads `originalCommit.oid` off a finding's comment to tell a + finding raised against THIS head from one raised against an OLDER head + (backend#2816). `originalCommit` is the commit the review was left on and it + stays pinned there; `commit` fast-forwards to the head whenever the line + still maps, so it reads equal to the head for a finding raised against an + older commit -- which is why this gate reads `originalCommit`, not `commit`, + and why a query edited to stop asking for it is a defect and not a + simplification. + + Were it dropped, every finding's `raised_against` would be None, which + fail-closed treats as against-this-head -- so the gate would silently revert + to fast-failing the post-fix-push race it was changed to WAIT through, and + the log would stay green while doing it. Same inert-guard shape as + `query_lacks_author_kind`, one field over: the run refuses rather than + reporting from a field it stopped requesting. Derived by reading the query. + """ + return re.search(r"originalCommit\s*\{[^}]*oid", query) is None + + def author_kind(pr): """Which kind of actor opened this PR: AUTHOR_BOT, AUTHOR_HUMAN, or None. @@ -573,6 +596,12 @@ def findings(pr): "severity": severity_of(body), "resolved": bool(thread.get("isResolved")), "outdated": bool(thread.get("isOutdated")), + # The commit the finding was RAISED AGAINST -- `originalCommit`, + # which stays pinned to the review's commit, never `commit`, + # which fast-forwards to the head (backend#2816). None when + # GitHub gave no commit, which evaluate() treats as + # against-this-head: fail closed, "cannot tell" blocks. + "raised_against": ((first.get("originalCommit") or {}) or {}).get("oid"), "title": (body.strip().splitlines() or ["(no title)"])[0].lstrip("# ").strip(), "url": first.get("url") or "", } @@ -668,25 +697,65 @@ def evaluate(pr, min_severity): check = bugbot_check(pr) claimed = check is not None and check.get("status") in TERMINAL_STATUSES - if not claimed and blocking: - head = (pr.get("headRefOid") or "?")[:12] + + # WHERE AN OUTSTANDING FINDING WAS RAISED IS THE DISCRIMINATOR (backend#2816). + # + # The fast-fail above still holds when the finding was raised against the + # head being evaluated: a completed review already saw this exact commit, so + # a verdict that is still coming cannot change the answer. It does NOT hold + # for the post-fix-push race, which is the ticket: the fix cycle is push -> + # reply -> resolve, the push fires this gate before the resolve can land, and + # the finding it sees was raised against the PREVIOUS head. Bugbot is now + # IN_PROGRESS on the new one and a verdict is seconds away, so this case must + # WAIT -- exactly as the gate already does when there are no findings at all. + # + # So the finding's `raised_against` commit (from findings(), read off + # `originalCommit`) is split against the head: + head_oid = pr.get("headRefOid") + against_this_head = [] + against_older_head = [] + for f in blocking: + # FAIL CLOSED: a finding whose commit could not be read (None) is treated + # as against THIS head and blocks. "Cannot tell" must never become + # permission to wait it out. + if f["raised_against"] is None or f["raised_against"] == head_oid: + against_this_head.append(f) + else: + against_older_head.append(f) + + # WHAT STILL FAST-FAILS, AND WHAT NOW WAITS. This is a NARROWING of the + # fast-fail, never a weakening: + # * IN_PROGRESS on this head (check is not None here, since `claimed` is + # false) -> only findings against THIS head fast-fail; findings against an + # older head fall through to PENDING and are decided by the verdict that + # is arriving (and still block at the deadline if it never does -- a + # stalled Bugbot still blocks). + # * NEVER CLAIMED (check is None) -> ANY blocking finding fast-fails, exactly + # as before. There is no in-progress verdict to wait for, and tolerating + # an older-head finding here would LAUNDER it across a dropped review -- + # the #356 order bug the block below the comprehension guards against. + fast_blockers = against_this_head if check is not None else blocking + if not claimed and fast_blockers: + head = (head_oid or "?")[:12] lines.append( "Bugbot %s head %s, AND %d open finding(s) at or above %s are " - "outstanding from an earlier review." + "outstanding that a pending verdict would not clear." % ( "never claimed" if check is None else "has not finished (status %s) on" % check.get("status"), head, - len(blocking), + len(fast_blockers), min_severity, ) ) lines.append( - "The absence of a review on THIS head is tolerated; the findings " - "are not. Resolve them -- fix, or reply with the ticket and resolve " - "-- then re-run. This does not wait for the missing review: the " - "answer would not change." + "A finding raised against THIS head (or one whose commit could not " + "be read) is not cleared by a pending review. Resolve it -- fix, or " + "reply with the ticket and resolve -- then re-run. (A finding raised " + "against an OLDER head, while Bugbot is IN_PROGRESS on this one, is " + "the post-fix-push race and is NOT counted here: that waits for the " + "verdict instead of failing -- backend#2816.)" ) lines.extend(_finding_lines(found)) return FAIL, lines @@ -700,13 +769,31 @@ def evaluate(pr, min_severity): "checked before this verdict, not after." % min_severity, ] if not claimed: - return PENDING, [ + pending_lines = [ "Bugbot is still running on head %s (status %s)." - % ((pr.get("headRefOid") or "?")[:12], check.get("status")), + % ((head_oid or "?")[:12], check.get("status")), "A verdict that has not arrived is not a clean one.", - "No open finding at or above %s from any earlier review, either -- " - "checked before this verdict, not after." % min_severity, ] + if against_older_head: + # The post-fix-push race (backend#2816): findings remain open, but + # every one was raised against an OLDER head, so we wait for the + # verdict on THIS head rather than fast-failing findings a resolve is + # about to clear. If Bugbot never finishes, PENDING still blocks at + # the deadline (main), so this defers the decision -- it does not + # drop it. + pending_lines.append( + "%d open finding(s) at or above %s remain, but every one was " + "raised against an OLDER head -- the post-fix-push race. Waiting " + "for the verdict on this head; if it never lands, this still " + "blocks at the deadline (backend#2816)." % (len(against_older_head), min_severity) + ) + pending_lines.extend(_finding_lines(found)) + else: + pending_lines.append( + "No open finding at or above %s from any earlier review, either " + "-- checked before this verdict, not after." % min_severity + ) + return PENDING, pending_lines lines.append( "Bugbot reviewed head %s -- check %r, status %s, conclusion %s." @@ -808,6 +895,18 @@ def main(argv=None): "Bot-authored one. See author_kind.") return 2 + # Same shape again (backend#2816): if the query stopped asking for a + # finding's originalCommit, every finding reads as raised against no commit, + # fail-closed folds them into against-this-head, and the head-scoped + # fast-fail silently reverts to blocking the race it was changed to wait + # through -- with a green run. A defect in this file, so it fails the run. + if query_lacks_finding_commit(): + _emit(FAIL, [], "the GraphQL query no longer requests originalCommit.oid " + "on finding comments, so a finding raised against an older " + "head cannot be told from one against this head. See " + "query_lacks_finding_commit.") + return 2 + sleeper = time.sleep clock = time.monotonic deadline = clock() + max(0, wait_seconds) diff --git a/scripts/tests/bugbot-gate-mutations.py b/scripts/tests/bugbot-gate-mutations.py index db23aca..f2405ec 100644 --- a/scripts/tests/bugbot-gate-mutations.py +++ b/scripts/tests/bugbot-gate-mutations.py @@ -107,15 +107,53 @@ # a gate that reached FAIL for some other reason entirely. ("the head is classified BEFORE the threshold, so a dropped review " "launders an open High", - ' if not claimed and blocking:', - ' if False and not claimed and blocking:'), + ' if not claimed and fast_blockers:', + ' if False and not claimed and fast_blockers:'), ("only the never-claimed absence checks the threshold, leaving PENDING " "with the same hole", - ' if not claimed and blocking:', - ' if check is None and blocking:'), + ' if not claimed and fast_blockers:', + ' if check is None and fast_blockers:'), ("the laundering guard fires on any absence, so the tolerance is gone", - ' if not claimed and blocking:', + ' if not claimed and fast_blockers:', ' if not claimed:'), + + # --- (A3b) HEAD-SCOPED FAST-FAIL (backend#2816) ------------------------- + # + # The fast-fail must distinguish WHERE an outstanding finding was raised. + # Each mutation here restores a shape that either re-breaks the post-fix-push + # race (an older-head finding fast-failing while a verdict is seconds away) + # or opens a laundering/fail-open hole, and is what makes the section-3b + # selftest cases load-bearing rather than decorative. + # + # The two one-way collapses of the fast-blocker set: + ("an older-head finding fast-fails while Bugbot re-reviews, so the race stays red", + ' fast_blockers = against_this_head if check is not None else blocking', + ' fast_blockers = blocking'), + ("a never-claimed head stops blocking an older-head finding, laundering it", + ' fast_blockers = against_this_head if check is not None else blocking', + ' fast_blockers = against_this_head'), + # Fail-closed removed: an unreadable finding-commit is filed as an OLDER head + # (so it waits) instead of THIS head (so it blocks). + ("an unreadable finding commit is treated as an older head, not fail-closed", + ' if f["raised_against"] is None or f["raised_against"] == head_oid:', + ' if f["raised_against"] == head_oid:'), + # The head comparison inverted: this-head and older-head swap wholesale. + ("the head comparison is inverted, so this-head and older-head swap", + ' if f["raised_against"] is None or f["raised_against"] == head_oid:', + ' if f["raised_against"] is None or f["raised_against"] != head_oid:'), + # findings() stops reading the commit at all: every finding becomes None -> + # fail-closed to this-head -> the race is fast-failed again, green log. + ("findings stops reading the finding's commit, so the race is fast-failed again", + ' "raised_against": ((first.get("originalCommit") or {}) or {}).get("oid"),', + ' "raised_against": None,'), + # The query stops asking for the commit -- caught by the integrity guard, the + # same shape as the totalCount and author.__typename guards. + ("the query stops asking which commit a finding was raised against", + ' originalCommit { oid }\n', + ''), + ("the finding-commit query guard is disarmed, so an inert head-scope runs green", + ' return re.search(r"originalCommit\\s*\\{[^}]*oid", query) is None', + ' return False'), # `blocking` is the threshold itself. If it collapses to "any open # finding", the split measured in backend#2284 dies -- a Low on an # unreviewed head would block, which is the thing the gate was loosened to diff --git a/scripts/tests/bugbot-gate-selftest.py b/scripts/tests/bugbot-gate-selftest.py index 22cd3ad..5f9ab9d 100644 --- a/scripts/tests/bugbot-gate-selftest.py +++ b/scripts/tests/bugbot-gate-selftest.py @@ -122,11 +122,19 @@ def finding_body(severity="High", title="A real bug", marker=True): return "\n".join(parts) -def thread(body, login="cursor", resolved=False, outdated=False): +def thread(body, login="cursor", resolved=False, outdated=False, raised_against=None): + # `raised_against` is the commit the finding was raised against, as GitHub + # returns it under `comments.nodes[0].originalCommit.oid`. Left ABSENT by + # default (not set to None-in-a-dict): the gate reads a missing/None commit + # as against-this-head and blocks -- fail closed -- and every pre-#2816 case + # here relies on that, so the default must reproduce "GitHub gave no commit". + comment = {"author": {"login": login}, "body": body, "url": "u"} + if raised_against is not None: + comment["originalCommit"] = {"oid": raised_against} return { "isResolved": resolved, "isOutdated": outdated, - "comments": {"nodes": [{"author": {"login": login}, "body": body, "url": "u"}]}, + "comments": {"nodes": [comment]}, } @@ -378,6 +386,75 @@ def pr(contexts=None, threads=None, head=HEAD, ctx_total=None, thread_total=None "got %r" % v, ) +# -------------------------------------------------------------------------- +# 3b. HEAD-SCOPED FAST-FAIL (backend#2816). The fast-fail must distinguish +# WHERE an outstanding finding was raised: against THIS head -> fail fast as +# before; against an OLDER head while Bugbot is IN_PROGRESS on this one (the +# post-fix-push race) -> WAIT for the verdict. A narrowing, never a +# weakening -- a finding against this head still blocks, a never-claimed +# head still blocks any finding (no laundering), and a stalled Bugbot still +# blocks (asserted through main, in the exit-code block below). +# -------------------------------------------------------------------------- +OLDER = "b" * 40 # a commit that is not HEAD -- an older review head + +# THE FIX: an open High raised against an OLDER head, Bugbot IN_PROGRESS on THIS +# head, WAITS instead of fast-failing. +v = ev(pr(contexts=[check_run(status="IN_PROGRESS", conclusion=None)], + threads=[thread(finding_body("High"), raised_against=OLDER)]), "high") +check("older-head High + IN_PROGRESS waits (PENDING), it does not fast-fail", + v == gate.PENDING, "got %r" % v) + +# ... and a High raised against THIS head still fails fast, even mid-review. +v = ev(pr(contexts=[check_run(status="IN_PROGRESS", conclusion=None)], + threads=[thread(finding_body("High"), raised_against=HEAD)]), "high") +check("this-head High + IN_PROGRESS still FAILs fast", v == gate.FAIL, "got %r" % v) + +# FAIL CLOSED: a finding whose commit could not be read (no originalCommit) is +# treated as against-this-head and blocks, even while Bugbot is running. "Cannot +# tell" must not become permission to wait it out. +v = ev(pr(contexts=[check_run(status="IN_PROGRESS", conclusion=None)], + threads=[thread(finding_body("High"))]), "high") # raised_against absent +check("unknown-commit High + IN_PROGRESS FAILs (fail closed)", v == gate.FAIL, "got %r" % v) + +# ANTI-LAUNDER, UNCHANGED: an older-head High with NO check on the head still +# fast-fails -- there is no in-progress verdict to wait for, so tolerating it +# would launder a finding across a dropped review (the #356 order bug). +v = ev(pr(contexts=[], threads=[thread(finding_body("High"), raised_against=OLDER)]), "high") +check("older-head High + NEVER-CLAIMED still FAILs (no laundering)", v == gate.FAIL, "got %r" % v) + +# A completed review on the head blocks on ANY open blocker, whichever head it +# was raised against -- the reviewed path is head-agnostic and must stay so. +v = ev(pr(contexts=[check_run()], threads=[thread(finding_body("High"), raised_against=OLDER)]), "high") +check("older-head High + COMPLETED review still FAILs (reviewed path)", v == gate.FAIL, "got %r" % v) + +# THE SPLIT MUST BE REAL: the same open older-head High yields OPPOSITE verdicts, +# decided solely by whether Bugbot is IN_PROGRESS (wait) or absent (block). A +# mutation that ignores the head classification collapses these two. +v_wait = ev(pr(contexts=[check_run(status="IN_PROGRESS", conclusion=None)], + threads=[thread(finding_body("High"), raised_against=OLDER)]), "high") +v_block = ev(pr(contexts=[], threads=[thread(finding_body("High"), raised_against=OLDER)]), "high") +check("the head-scope split is real: IN_PROGRESS waits, absent blocks", + v_wait == gate.PENDING and v_block == gate.FAIL and v_wait != v_block, + "in_progress=%r absent=%r" % (v_wait, v_block)) + +# A this-head finding is NOT deferred just because an older-head one shares the +# PR: the this-head blocker dominates the fast-fail mid-review. +v = ev(pr(contexts=[check_run(status="IN_PROGRESS", conclusion=None)], + threads=[thread(finding_body("High"), raised_against=OLDER), + thread(finding_body("High"), raised_against=HEAD)]), "high") +check("a this-head High still fast-fails alongside an older-head one mid-review", + v == gate.FAIL, "got %r" % v) + +# THE QUERY-INTEGRITY GUARD, both directions, inputs written independently of +# the module (rule 9's corollary) and mirroring the author.__typename guard. +check("the live query DOES ask for the finding's originalCommit", + gate.query_lacks_finding_commit() is False) +check("a query that omits originalCommit is caught", + gate.query_lacks_finding_commit("{ comments { nodes { body url } } }") is True) +check("a query that asks for originalCommit.oid is not caught", + gate.query_lacks_finding_commit( + "{ comments { nodes { originalCommit { oid } } } }") is False) + # -------------------------------------------------------------------------- # 4. What is, and is not, a finding. # -------------------------------------------------------------------------- @@ -686,6 +763,27 @@ def _banner_for(pr_obj): threads=[thread(finding_body("High"), resolved=False)])) check("main: an open finding still exits 1", rc_fail == 1, "got rc=%r" % rc_fail) + # backend#2816 (c): A STALLED Bugbot STILL BLOCKS. Bugbot is IN_PROGRESS + # on this head with only an OLDER-head finding open; WAIT_SECONDS=0 makes + # the deadline already past on the first pass, so this is the stall, not + # the race resolving. Head-scoping defers it to PENDING (rather than the + # old fast-fail FAIL), and PENDING blocks at the deadline -- same exit + # code 1, so both `.github#383` stall cases still fail. Without this, the + # narrowing could have turned a stall into an exit-0 wait. + rc_stall = _main_rc( + pr(contexts=[check_run(status="IN_PROGRESS", conclusion=None)], + threads=[thread(finding_body("High"), resolved=False, + raised_against="b" * 40)])) + check("main: older-head High + STALLED (IN_PROGRESS past budget) exits 1", + rc_stall == 1, "got rc=%r" % rc_stall) + # And the race, at the deadline, must not have become a silent pass: a + # never-claimed head carrying an older-head High still exits 1, never 0. + rc_older_unclaimed = _main_rc( + pr(contexts=[], threads=[thread(finding_body("High"), resolved=False, + raised_against="b" * 40)])) + check("main: older-head High + never-claimed head exits 1, not 0", + rc_older_unclaimed == 1, "got rc=%r" % rc_older_unclaimed) + # ------------------------------------------------------------------- # THE TOLERANCE MUST NOT LAUNDER A FINDING (Bugbot, #356). #