From 33ada7c48f124715c25bfa4b0da8d53e19c5c934 Mon Sep 17 00:00:00 2001 From: Sinity Date: Sat, 1 Aug 2026 15:33:46 +0200 Subject: [PATCH 1/3] feat(devtools): add merge-gate, a structural pre-merge safety check Problem: across a ~28-PR merge train in one coordinator session (2026-08-01), two incidents showed operator memory doesn't scale as the sole safety net before squash-merging. PR #3502 merged with 0 review comments showing at check time; CodeRabbit posted 3 real findings 30-60s later (caught only by an ad hoc grace-period poll habit adopted afterward). PR #3517 nearly merged carrying a 43-test regression that no CI check or review comment ever flagged, since per-PR CI deliberately skips the heavy test suite (CLAUDE.md) - caught only because the coordinator happened to run the broader local suite by hand before merging that specific time. What changed: `devtools workspace merge-gate record --command "..."` runs a local verification command against a PR's current head sha and persists a receipt keyed to that exact sha under .cache/verify/merge-gate/. `merge-gate check ` BLOCKs unless a fresh, exit-0 receipt exists for the PR's CURRENT head (a new push invalidates the old receipt) and no review comment's created_at is newer than the head commit's timestamp - late comments are listed explicitly rather than requiring a human to compare two timestamps by hand. This doesn't replace judgment about what a late comment means; it makes an unverified late signal impossible to merge past silently. Verification: devtools test tests/unit/devtools/test_merge_gate.py (9 passed, covering fresh/stale-sha/late-comment/closed-PR/expired- receipt cases against a faked gh subprocess). Live smoke test against open PR #3517: record + check round-tripped correctly (BLOCK before recording, OK after). devtools verify --quick exit 0. --- devtools/command_catalog.py | 22 +++ devtools/merge_gate.py | 235 +++++++++++++++++++++++++ docs/devtools.md | 1 + tests/unit/devtools/test_merge_gate.py | 156 ++++++++++++++++ 4 files changed, 414 insertions(+) create mode 100644 devtools/merge_gate.py create mode 100644 tests/unit/devtools/test_merge_gate.py diff --git a/devtools/command_catalog.py b/devtools/command_catalog.py index d483254b11..fe00130761 100644 --- a/devtools/command_catalog.py +++ b/devtools/command_catalog.py @@ -679,6 +679,28 @@ def to_dict(self) -> dict[str, object]: "devtools workspace verify-worktree /realm/worktrees/lane-x --json --strict", ), ), + CommandSpec( + "workspace merge-gate", + "workspace", + "Structural pre-merge safety check: fresh local-verification receipt + no late review comments.", + "devtools.merge_gate", + use_when=( + "Immediately before squash-merging any PR in a merge train, replacing coordinator memory " + "(grace-period comment polling, remembering to run the broader local test suite CI skips " + 'per-PR) with a check that fails closed. `record --command "..."` runs a local ' + "verification command against the PR's current head sha and persists a receipt; " + "`check ` BLOCKs unless a receipt exists for the CURRENT head sha within a freshness " + "window with exit_code 0, and no review comment's created_at is newer than the head " + "commit's timestamp (printed explicitly, not silently skipped). Motivated by two " + "2026-08-01 incidents: PR #3502 merged before CodeRabbit's findings posted, and PR #3517 " + "nearly merged with a 43-test regression no CI check or review comment ever flagged." + ), + examples=( + 'devtools workspace merge-gate record 3517 --command "devtools verify --quick"', + "devtools workspace merge-gate check 3517", + "devtools workspace merge-gate check 3517 --json --max-age-s 7200", + ), + ), CommandSpec( "workspace merge-conductor", "workspace", diff --git a/devtools/merge_gate.py b/devtools/merge_gate.py new file mode 100644 index 0000000000..8df6c668c4 --- /dev/null +++ b/devtools/merge_gate.py @@ -0,0 +1,235 @@ +"""merge-gate: make "is this PR actually safe to merge" a structural check, not a memory habit. + +Two real incidents from a single ~28-PR merge-train session (2026-08-01) motivate +this: + + 1. PR #3502 was squash-merged with 0 review comments showing at check time; + CodeRabbit posted 3 real findings 30-60s later. The fix (an explicit + "poll comments a few times before merging" habit) worked for the rest of + that session, but it lived entirely in the coordinator's own discipline. + 2. PR #3517 nearly merged carrying a 43-test regression that no CI check and + no review comment ever flagged -- per-PR CI deliberately skips the heavy + test suite (see CLAUDE.md), so nothing but a coordinator choosing, from + memory, to run the broader local suite before merging would have caught + it. It was caught, that time. + +A coordinator merging dozens of PRs across a few hours cannot reliably repeat +either habit purely from memory every single time. This command turns both +into something that fails closed instead of silently not happening: + + - ``record``: run (or accept the exit code of) a local verification command + against a PR branch's current HEAD commit, and persist a receipt keyed to + that exact sha under ``.cache/verify/merge-gate/pr-.json``. + - ``check``: BLOCK unless a receipt exists for the PR's *current* head sha + (not a stale one from an earlier push), was recorded within a freshness + window, and had exit code 0 -- AND no review comment's ``created_at`` is + newer than that head commit's ``committedDate``. Late-arriving comments + are printed explicitly rather than requiring a human to eyeball two + timestamps; a push after the last recorded receipt is a hard block, not + an advisory. + +This does not replace judgment about *what* a late comment means -- it makes +the presence of an unverified late signal impossible to merge past silently. + +Usage: + devtools workspace merge-gate record 3517 --command "devtools verify --quick" + devtools workspace merge-gate check 3517 + devtools workspace merge-gate check 3517 --json --max-age-s 7200 +""" + +from __future__ import annotations + +import argparse +import json +import shlex +import subprocess +import sys +import time +from dataclasses import asdict, dataclass, field +from pathlib import Path +from typing import Any + +_RECEIPT_DIR = Path(".cache/verify/merge-gate") +_DEFAULT_MAX_AGE_S = 3600 + + +def _gh_json(args: list[str]) -> Any: + result = subprocess.run(["gh", *args], capture_output=True, text=True, timeout=60) + if result.returncode != 0: + raise RuntimeError(result.stderr.strip()[:300] or f"gh {' '.join(args)} failed") + return json.loads(result.stdout) + + +@dataclass +class GateVerdict: + pr: int + ok: bool + reasons: list[str] = field(default_factory=list) + head_sha: str = "" + receipt: dict[str, Any] | None = None + late_comments: list[dict[str, Any]] = field(default_factory=list) + + +def _receipt_path(pr: int) -> Path: + return _RECEIPT_DIR / f"pr-{pr}.json" + + +def cmd_record(pr: int, command: str) -> int: + info = _gh_json(["pr", "view", str(pr), "--json", "headRefOid,headRefName"]) + head_sha = info["headRefOid"] + + argv = shlex.split(command) + started = time.time() + result = subprocess.run(argv, capture_output=True, text=True) + duration_s = round(time.time() - started, 2) + + receipt = { + "pr": pr, + "head_sha": head_sha, + "branch": info["headRefName"], + "command": command, + "exit_code": result.returncode, + "duration_s": duration_s, + "recorded_at": time.time(), + "stdout_tail": result.stdout[-4000:], + "stderr_tail": result.stderr[-4000:], + } + _RECEIPT_DIR.mkdir(parents=True, exist_ok=True) + _receipt_path(pr).write_text(json.dumps(receipt, indent=2)) + + print(f"recorded receipt for PR #{pr} @ {head_sha[:8]}: exit={result.returncode} ({duration_s}s)") + if result.returncode != 0: + print(result.stdout[-2000:]) + print(result.stderr[-2000:], file=sys.stderr) + return result.returncode + + +def cmd_check(pr: int, *, max_age_s: int, as_json: bool) -> int: + verdict = GateVerdict(pr=pr, ok=True) + + try: + info = _gh_json( + [ + "pr", + "view", + str(pr), + "--json", + "headRefOid,mergeStateStatus,state,commits", + ] + ) + except (RuntimeError, json.JSONDecodeError, OSError, subprocess.SubprocessError) as exc: + verdict.ok = False + verdict.reasons.append(f"gh pr view failed: {exc}") + _emit(verdict, as_json) + return 1 + + if info.get("state") != "OPEN": + verdict.ok = False + verdict.reasons.append(f"PR state is {info.get('state')!r}, not OPEN") + _emit(verdict, as_json) + return 1 + + head_sha = info["headRefOid"] + verdict.head_sha = head_sha + + mss = info.get("mergeStateStatus", "") + if mss not in {"CLEAN", "UNSTABLE", "UNKNOWN"}: + verdict.ok = False + verdict.reasons.append(f"mergeStateStatus is {mss!r} (expected CLEAN/UNSTABLE/UNKNOWN)") + + commits = info.get("commits") or [] + head_commit = commits[-1] if commits else None + head_committed_at = head_commit.get("committedDate") if head_commit else None + + receipt_path = _receipt_path(pr) + if not receipt_path.exists(): + verdict.ok = False + verdict.reasons.append( + f"no local verification receipt found at {receipt_path} -- run " + f'`devtools workspace merge-gate record {pr} --command "..."` against the current head first' + ) + else: + receipt = json.loads(receipt_path.read_text()) + verdict.receipt = receipt + if receipt.get("head_sha") != head_sha: + verdict.ok = False + verdict.reasons.append( + f"receipt is for sha {receipt.get('head_sha', '')[:8]} but PR head is now {head_sha[:8]} " + "-- a new commit landed since the receipt was recorded; re-record before merging" + ) + else: + age_s = time.time() - receipt.get("recorded_at", 0) + if age_s > max_age_s: + verdict.ok = False + verdict.reasons.append(f"receipt is {int(age_s)}s old (max {max_age_s}s) -- re-record") + if receipt.get("exit_code", 1) != 0: + verdict.ok = False + verdict.reasons.append(f"receipt exit_code is {receipt.get('exit_code')}, not 0") + + try: + review_comments = _gh_json(["api", f"repos/{{owner}}/{{repo}}/pulls/{pr}/comments"]) + except (RuntimeError, json.JSONDecodeError, OSError, subprocess.SubprocessError) as exc: + verdict.ok = False + verdict.reasons.append(f"could not fetch review comments: {exc}") + review_comments = [] + + if head_committed_at: + for comment in review_comments: + created_at = comment.get("created_at", "") + if created_at > head_committed_at: + verdict.late_comments.append( + { + "path": comment.get("path"), + "line": comment.get("line"), + "created_at": created_at, + "body_head": (comment.get("body") or "")[:200], + } + ) + if verdict.late_comments: + verdict.ok = False + verdict.reasons.append( + f"{len(verdict.late_comments)} review comment(s) posted after the head commit " + f"({head_committed_at}) -- read and triage before merging" + ) + elif review_comments: + verdict.reasons.append("could not determine head commit timestamp; late-comment check skipped") + + _emit(verdict, as_json) + return 0 if verdict.ok else 1 + + +def _emit(verdict: GateVerdict, as_json: bool) -> None: + if as_json: + print(json.dumps(asdict(verdict), indent=2)) + return + print(f"PR #{verdict.pr} @ {verdict.head_sha[:8] if verdict.head_sha else '?'}: {'OK' if verdict.ok else 'BLOCK'}") + for reason in verdict.reasons: + print(f" - {reason}") + for late in verdict.late_comments: + print(f" late comment [{late['path']}:{late['line']}] {late['created_at']}: {late['body_head']}") + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter) + sub = parser.add_subparsers(dest="action", required=True) + + record_p = sub.add_parser("record", help="Run local verification against a PR's current head and persist a receipt") + record_p.add_argument("pr", type=int) + record_p.add_argument( + "--command", required=True, help="Local verification command to run, e.g. 'devtools verify --quick'" + ) + + check_p = sub.add_parser("check", help="Decide whether a PR is safe to merge right now") + check_p.add_argument("pr", type=int) + check_p.add_argument("--max-age-s", type=int, default=_DEFAULT_MAX_AGE_S) + check_p.add_argument("--json", action="store_true", dest="as_json") + + args = parser.parse_args(argv) + + if args.action == "record": + return cmd_record(args.pr, args.command) + return cmd_check(args.pr, max_age_s=args.max_age_s, as_json=args.as_json) + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/docs/devtools.md b/docs/devtools.md index 145b763a6e..ffcfc579bb 100644 --- a/docs/devtools.md +++ b/docs/devtools.md @@ -233,6 +233,7 @@ These are the commands worth remembering during normal repo work: | `devtools workspace lineage-validation` | Validate lineage-count evidence before citing archive counts externally. | | `devtools workspace mandate-continuity-replay` | Wire t8t continuity scenarios + work-evidence effects + discovery into one mandate artifact. | | `devtools workspace merge-conductor` | Mechanical-conflict triage for the PR merge train (dry-run by default). | +| `devtools workspace merge-gate` | Structural pre-merge safety check: fresh local-verification receipt + no late review comments. | | `devtools workspace raw-authority-daemon-health-proof` | Prove daemon status/health HTTP responsiveness during a real raw-authority drain. | | `devtools workspace raw-authority-restart-proof` | Prove raw-authority crash recovery and conserved fixed-point convergence. | | `devtools workspace raw-authority-scale-proof` | Run bounded raw-authority replay to a two-census fixed point. | diff --git a/tests/unit/devtools/test_merge_gate.py b/tests/unit/devtools/test_merge_gate.py new file mode 100644 index 0000000000..00a10b28f7 --- /dev/null +++ b/tests/unit/devtools/test_merge_gate.py @@ -0,0 +1,156 @@ +from __future__ import annotations + +import json +import subprocess +from pathlib import Path +from unittest.mock import MagicMock + +import pytest + +from devtools import merge_gate + + +def _fake_run(pr_view: dict[str, object], comments: list[dict[str, object]], local_exit: int = 0) -> object: + def _run(cmd: list[str], **kwargs: object) -> MagicMock: + if cmd[:3] == ["gh", "pr", "view"]: + return MagicMock(returncode=0, stdout=json.dumps(pr_view), stderr="") + if cmd[:2] == ["gh", "api"]: + return MagicMock(returncode=0, stdout=json.dumps(comments), stderr="") + return MagicMock(returncode=local_exit, stdout="ok\n", stderr="") + + return _run + + +def test_record_persists_receipt_keyed_to_current_head_sha(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + monkeypatch.chdir(tmp_path) + monkeypatch.setattr( + subprocess, + "run", + _fake_run({"headRefOid": "abc123", "headRefName": "feature/x"}, []), + ) + + exit_code = merge_gate.cmd_record(42, "true") + + assert exit_code == 0 + receipt = json.loads(merge_gate._receipt_path(42).read_text()) + assert receipt["head_sha"] == "abc123" + assert receipt["exit_code"] == 0 + + +def test_record_captures_nonzero_local_command_exit(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + monkeypatch.chdir(tmp_path) + monkeypatch.setattr( + subprocess, + "run", + _fake_run({"headRefOid": "abc123", "headRefName": "feature/x"}, [], local_exit=1), + ) + + exit_code = merge_gate.cmd_record(42, "false") + + assert exit_code == 1 + receipt = json.loads(merge_gate._receipt_path(42).read_text()) + assert receipt["exit_code"] == 1 + + +def _base_pr_view(head_sha: str = "abc123", committed_date: str = "2026-08-01T12:00:00Z") -> dict[str, object]: + return { + "headRefOid": head_sha, + "headRefName": "feature/x", + "state": "OPEN", + "mergeStateStatus": "CLEAN", + "commits": [{"oid": head_sha, "committedDate": committed_date}], + } + + +def test_check_blocks_when_no_receipt_exists(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + monkeypatch.chdir(tmp_path) + monkeypatch.setattr(subprocess, "run", _fake_run(_base_pr_view(), [])) + + exit_code = merge_gate.cmd_check(42, max_age_s=3600, as_json=False) + + assert exit_code == 1 + + +def test_check_ok_when_receipt_fresh_and_matches_head_with_no_late_comments( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + monkeypatch.chdir(tmp_path) + monkeypatch.setattr(subprocess, "run", _fake_run(_base_pr_view(), [])) + merge_gate.cmd_record(42, "true") + + exit_code = merge_gate.cmd_check(42, max_age_s=3600, as_json=False) + + assert exit_code == 0 + + +def test_check_blocks_when_receipt_is_for_a_stale_sha(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + monkeypatch.chdir(tmp_path) + monkeypatch.setattr(subprocess, "run", _fake_run(_base_pr_view(head_sha="abc123"), [])) + merge_gate.cmd_record(42, "true") + + # A new commit landed after the receipt was recorded. + monkeypatch.setattr(subprocess, "run", _fake_run(_base_pr_view(head_sha="def456"), [])) + exit_code = merge_gate.cmd_check(42, max_age_s=3600, as_json=False) + + assert exit_code == 1 + + +def test_check_blocks_on_review_comment_newer_than_head_commit(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + monkeypatch.chdir(tmp_path) + pr_view = _base_pr_view(committed_date="2026-08-01T12:00:00Z") + late_comment = [ + { + "path": "polylogue/foo.py", + "line": 10, + "created_at": "2026-08-01T12:05:00Z", + "body": "this is a real finding", + } + ] + monkeypatch.setattr(subprocess, "run", _fake_run(pr_view, [])) + merge_gate.cmd_record(42, "true") + + monkeypatch.setattr(subprocess, "run", _fake_run(pr_view, late_comment)) + exit_code = merge_gate.cmd_check(42, max_age_s=3600, as_json=False) + + assert exit_code == 1 + + +def test_check_ignores_comment_older_than_head_commit(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + monkeypatch.chdir(tmp_path) + pr_view = _base_pr_view(committed_date="2026-08-01T12:00:00Z") + stale_comment = [ + { + "path": "polylogue/foo.py", + "line": 10, + "created_at": "2026-08-01T11:55:00Z", + "body": "already addressed by the fix commit", + } + ] + monkeypatch.setattr(subprocess, "run", _fake_run(pr_view, [])) + merge_gate.cmd_record(42, "true") + + monkeypatch.setattr(subprocess, "run", _fake_run(pr_view, stale_comment)) + exit_code = merge_gate.cmd_check(42, max_age_s=3600, as_json=False) + + assert exit_code == 0 + + +def test_check_blocks_when_pr_is_not_open(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + monkeypatch.chdir(tmp_path) + pr_view = _base_pr_view() + pr_view["state"] = "MERGED" + monkeypatch.setattr(subprocess, "run", _fake_run(pr_view, [])) + + exit_code = merge_gate.cmd_check(42, max_age_s=3600, as_json=False) + + assert exit_code == 1 + + +def test_check_blocks_when_receipt_older_than_max_age(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + monkeypatch.chdir(tmp_path) + monkeypatch.setattr(subprocess, "run", _fake_run(_base_pr_view(), [])) + merge_gate.cmd_record(42, "true") + + exit_code = merge_gate.cmd_check(42, max_age_s=-1, as_json=False) + + assert exit_code == 1 From 8f450850f5c87f1eb3171dee1cce57f4eca7122b Mon Sep 17 00:00:00 2001 From: Sinity Date: Sat, 1 Aug 2026 15:49:41 +0200 Subject: [PATCH 2/3] fix(devtools): harden merge-gate against 4 review-found gaps CodeRabbit review on merge-gate's own PR (#3518) found real gaps: 1. record() only copied the fetched head_sha into the receipt without verifying the local checkout was actually AT that commit -- a receipt could attest to unrelated code (e.g. recording from master or a stale worktree). Now refuses (exit 2) unless `git rev-parse HEAD` matches the PR's headRefOid and the tree is clean. 2. The documented example used `devtools verify --quick`, which explicitly skips tests -- the exact profile that would have missed PR #3517's 43-test regression, this tool's own motivating incident. Fixed the example to `devtools verify`, and record() now tags a receipt with skips_tests: true when the command looks like it didn't run tests (heuristic), which check() surfaces as an advisory. 3. check() took a single comment snapshot, so a comment posted 30-60s later (the PR #3502 incident this tool exists to prevent) could still slip through if check() ran before it landed. check() now polls comments across a configurable grace window (default 3x20s) instead of one snapshot. 4. Once a comment's created_at was later than the head commit, it blocked forever with no way to mark it triaged short of an empty commit. Added `ack --reason "..."`, scoped to the PR's current head sha so a new push always re-requires triage. Verification: devtools test tests/unit/devtools/test_merge_gate.py -- 16 passed (added: checkout-mismatch refusal, dirty-tree refusal, skips_tests flagging, multi-round poll catching a comment that only appears on round 2, ack suppressing a late comment for its exact head sha but not a different one). devtools verify --quick exit 0. --- devtools/command_catalog.py | 29 ++-- devtools/merge_gate.py | 207 ++++++++++++++++++++----- tests/unit/devtools/test_merge_gate.py | 204 +++++++++++++++++++++--- 3 files changed, 369 insertions(+), 71 deletions(-) diff --git a/devtools/command_catalog.py b/devtools/command_catalog.py index fe00130761..eda3bba122 100644 --- a/devtools/command_catalog.py +++ b/devtools/command_catalog.py @@ -687,18 +687,25 @@ def to_dict(self) -> dict[str, object]: use_when=( "Immediately before squash-merging any PR in a merge train, replacing coordinator memory " "(grace-period comment polling, remembering to run the broader local test suite CI skips " - 'per-PR) with a check that fails closed. `record --command "..."` runs a local ' - "verification command against the PR's current head sha and persists a receipt; " - "`check ` BLOCKs unless a receipt exists for the CURRENT head sha within a freshness " - "window with exit_code 0, and no review comment's created_at is newer than the head " - "commit's timestamp (printed explicitly, not silently skipped). Motivated by two " - "2026-08-01 incidents: PR #3502 merged before CodeRabbit's findings posted, and PR #3517 " - "nearly merged with a 43-test regression no CI check or review comment ever flagged." - ), - examples=( - 'devtools workspace merge-gate record 3517 --command "devtools verify --quick"', + 'per-PR) with a check that fails closed. `record --command "..."` checks out the ' + "PR's exact head commit (refuses on any mismatch or dirty tree), runs a local verification " + "command, and persists a receipt flagging commands that look like they skip tests (e.g. " + "`verify --quick`). `check ` polls review comments across a real grace window (default " + "3x20s, covering CodeRabbit's 30-60s late-arrival window) and BLOCKs unless a receipt exists " + "for the CURRENT head sha within a freshness window with exit_code 0, and no review comment's " + "created_at is newer than the head commit's timestamp unless explicitly `ack`'d for that exact " + "head sha. Motivated by two 2026-08-01 incidents: PR #3502 merged before CodeRabbit's findings " + "posted, and PR #3517 nearly merged with a 43-test regression no CI check or review comment " + "ever flagged -- plus review findings on this tool itself (recording from an unrelated " + "checkout, a --quick example that would have missed its own motivating regression, a single " + "comment snapshot instead of a grace-period poll, and no way to triage a false-positive late " + "comment without an empty commit)." + ), + examples=( + 'devtools workspace merge-gate record 3517 --command "devtools verify"', "devtools workspace merge-gate check 3517", - "devtools workspace merge-gate check 3517 --json --max-age-s 7200", + "devtools workspace merge-gate check 3517 --json --max-age-s 7200 --poll-rounds 1", + 'devtools workspace merge-gate ack 3517 123456789 --reason "already fixed upstream, false positive"', ), ), CommandSpec( diff --git a/devtools/merge_gate.py b/devtools/merge_gate.py index 8df6c668c4..18e89db250 100644 --- a/devtools/merge_gate.py +++ b/devtools/merge_gate.py @@ -17,24 +17,39 @@ either habit purely from memory every single time. This command turns both into something that fails closed instead of silently not happening: - - ``record``: run (or accept the exit code of) a local verification command - against a PR branch's current HEAD commit, and persist a receipt keyed to - that exact sha under ``.cache/verify/merge-gate/pr-.json``. - - ``check``: BLOCK unless a receipt exists for the PR's *current* head sha - (not a stale one from an earlier push), was recorded within a freshness - window, and had exit code 0 -- AND no review comment's ``created_at`` is - newer than that head commit's ``committedDate``. Late-arriving comments - are printed explicitly rather than requiring a human to eyeball two - timestamps; a push after the last recorded receipt is a hard block, not - an advisory. - -This does not replace judgment about *what* a late comment means -- it makes -the presence of an unverified late signal impossible to merge past silently. + - ``record``: run a local verification command against a PR branch's + current HEAD commit, and persist a receipt keyed to that exact sha under + ``.cache/verify/merge-gate/pr-.json``. Refuses to record unless the + CURRENT git checkout is clean and its ``HEAD`` matches the PR's fetched + ``headRefOid`` -- otherwise the receipt would attest to code that was + never actually tested (review-caught gap: recording from an unrelated + checkout, e.g. master or a stale worktree, previously produced a receipt + that ``check`` would accept). + - ``check``: polls PR review comments across a real grace window (default + 3 rounds x 20s, covering the 30-60s late-arrival window from incident 1) + before deciding. BLOCKs unless a receipt exists for the PR's *current* + head sha (not a stale one from an earlier push), was recorded within a + freshness window, had exit code 0, and its command actually looks like it + ran tests (a bare ``--quick`` profile is flagged, not silently accepted -- + review-caught gap: the documented example used exactly the profile that + would have missed the PR #3517 regression). No review comment's + ``created_at`` may be newer than the head commit's ``committedDate`` + unless it has been explicitly acknowledged via ``ack`` for this exact + head sha (review-caught gap: without ``ack``, a stale-forever comparison + made even a reviewed false positive permanently unmergeable without an + empty commit). + +This does not replace judgment about *what* a late comment means -- ``ack`` +still requires a human/agent to have actually read it and decided it's not +actionable. It makes the presence of an unverified late signal impossible to +merge past silently, and impossible to permanently paper over without an +explicit, current-head-scoped decision. Usage: - devtools workspace merge-gate record 3517 --command "devtools verify --quick" + devtools workspace merge-gate record 3517 --command "devtools verify" devtools workspace merge-gate check 3517 - devtools workspace merge-gate check 3517 --json --max-age-s 7200 + devtools workspace merge-gate check 3517 --json --max-age-s 7200 --poll-rounds 1 + devtools workspace merge-gate ack 3517 --reason "false positive, already fixed upstream" """ from __future__ import annotations @@ -51,6 +66,15 @@ _RECEIPT_DIR = Path(".cache/verify/merge-gate") _DEFAULT_MAX_AGE_S = 3600 +_DEFAULT_POLL_ROUNDS = 3 +_DEFAULT_POLL_INTERVAL_S = 20 +# Heuristic: profiles that explicitly skip tests (see CLAUDE.md -- `devtools +# verify --quick` is format+lint+mypy+render, no pytest). Not exhaustive; a +# command containing neither this nor an obvious test-runner name still gets +# flagged as an advisory, since the whole point is not trusting a plausible- +# looking command string without comment. +_TEST_SKIPPING_MARKERS: tuple[str, ...] = ("verify --quick", "verify --lab") +_LOOKS_LIKE_TESTS_MARKERS: tuple[str, ...] = ("test", "pytest", "verify --all") def _gh_json(args: list[str]) -> Any: @@ -60,6 +84,18 @@ def _gh_json(args: list[str]) -> Any: return json.loads(result.stdout) +def _git_head_sha() -> str | None: + result = subprocess.run(["git", "rev-parse", "HEAD"], capture_output=True, text=True, timeout=15) + if result.returncode != 0: + return None + return result.stdout.strip() + + +def _git_is_clean() -> bool: + result = subprocess.run(["git", "status", "--porcelain"], capture_output=True, text=True, timeout=15) + return result.returncode == 0 and not result.stdout.strip() + + @dataclass class GateVerdict: pr: int @@ -74,10 +110,39 @@ def _receipt_path(pr: int) -> Path: return _RECEIPT_DIR / f"pr-{pr}.json" +def _ack_path(pr: int) -> Path: + return _RECEIPT_DIR / f"pr-{pr}-acks.json" + + +def _command_skips_tests(command: str) -> bool: + lowered = command.lower() + if any(marker in lowered for marker in _TEST_SKIPPING_MARKERS): + return True + return not any(marker in lowered for marker in _LOOKS_LIKE_TESTS_MARKERS) + + def cmd_record(pr: int, command: str) -> int: info = _gh_json(["pr", "view", str(pr), "--json", "headRefOid,headRefName"]) head_sha = info["headRefOid"] + local_head = _git_head_sha() + if local_head != head_sha: + print( + f"REFUSING to record: current checkout HEAD ({local_head[:8] if local_head else '?'}) does not " + f"match PR #{pr}'s head ({head_sha[:8]}). Check out the PR's exact commit (in an isolated worktree " + "if other work is in progress elsewhere) before recording -- a receipt attesting to the wrong " + "checkout is worse than no receipt.", + file=sys.stderr, + ) + return 2 + if not _git_is_clean(): + print( + "REFUSING to record: current checkout has uncommitted changes. The receipt must attest to " + "exactly the PR's committed content, not a locally-modified tree.", + file=sys.stderr, + ) + return 2 + argv = shlex.split(command) started = time.time() result = subprocess.run(argv, capture_output=True, text=True) @@ -88,6 +153,7 @@ def cmd_record(pr: int, command: str) -> int: "head_sha": head_sha, "branch": info["headRefName"], "command": command, + "skips_tests": _command_skips_tests(command), "exit_code": result.returncode, "duration_s": duration_s, "recorded_at": time.time(), @@ -98,13 +164,54 @@ def cmd_record(pr: int, command: str) -> int: _receipt_path(pr).write_text(json.dumps(receipt, indent=2)) print(f"recorded receipt for PR #{pr} @ {head_sha[:8]}: exit={result.returncode} ({duration_s}s)") + if receipt["skips_tests"]: + print( + f" advisory: command {command!r} does not look like it ran tests -- `check` will flag this", + file=sys.stderr, + ) if result.returncode != 0: print(result.stdout[-2000:]) print(result.stderr[-2000:], file=sys.stderr) return result.returncode -def cmd_check(pr: int, *, max_age_s: int, as_json: bool) -> int: +def cmd_ack(pr: int, comment_id: int, *, reason: str) -> int: + info = _gh_json(["pr", "view", str(pr), "--json", "headRefOid"]) + head_sha = info["headRefOid"] + + ack_path = _ack_path(pr) + acks: dict[str, Any] = json.loads(ack_path.read_text()) if ack_path.exists() else {} + acks[str(comment_id)] = {"head_sha": head_sha, "reason": reason, "acked_at": time.time()} + _RECEIPT_DIR.mkdir(parents=True, exist_ok=True) + ack_path.write_text(json.dumps(acks, indent=2)) + print(f"acknowledged comment {comment_id} on PR #{pr} @ {head_sha[:8]}: {reason}") + return 0 + + +def _fetch_review_comments(pr: int) -> list[dict[str, Any]] | None: + try: + result: list[dict[str, Any]] = _gh_json(["api", f"repos/{{owner}}/{{repo}}/pulls/{pr}/comments"]) + except (RuntimeError, json.JSONDecodeError, OSError, subprocess.SubprocessError): + return None + return result + + +def _poll_stable_comments(pr: int, *, rounds: int, interval_s: int) -> list[dict[str, Any]] | None: + """Poll review comments repeatedly so a comment posted 30-60s after CI + goes green (the PR #3502 incident) is observed rather than missed by a + single snapshot taken too early.""" + last: list[dict[str, Any]] | None = None + for round_index in range(max(1, rounds)): + comments = _fetch_review_comments(pr) + if comments is None: + return None + last = comments + if round_index < rounds - 1: + time.sleep(interval_s) + return last + + +def cmd_check(pr: int, *, max_age_s: int, poll_rounds: int, poll_interval_s: int, as_json: bool) -> int: verdict = GateVerdict(pr=pr, ok=True) try: @@ -165,31 +272,44 @@ def cmd_check(pr: int, *, max_age_s: int, as_json: bool) -> int: if receipt.get("exit_code", 1) != 0: verdict.ok = False verdict.reasons.append(f"receipt exit_code is {receipt.get('exit_code')}, not 0") + if receipt.get("skips_tests"): + verdict.reasons.append( + f"advisory: receipt command {receipt.get('command')!r} does not look like it ran tests " + "-- confirm this PR genuinely needs no test coverage before merging" + ) - try: - review_comments = _gh_json(["api", f"repos/{{owner}}/{{repo}}/pulls/{pr}/comments"]) - except (RuntimeError, json.JSONDecodeError, OSError, subprocess.SubprocessError) as exc: + review_comments = _poll_stable_comments(pr, rounds=poll_rounds, interval_s=poll_interval_s) + if review_comments is None: verdict.ok = False - verdict.reasons.append(f"could not fetch review comments: {exc}") + verdict.reasons.append("could not fetch review comments after polling") review_comments = [] + ack_path = _ack_path(pr) + acks: dict[str, Any] = json.loads(ack_path.read_text()) if ack_path.exists() else {} + if head_committed_at: for comment in review_comments: created_at = comment.get("created_at", "") - if created_at > head_committed_at: - verdict.late_comments.append( - { - "path": comment.get("path"), - "line": comment.get("line"), - "created_at": created_at, - "body_head": (comment.get("body") or "")[:200], - } - ) + if created_at <= head_committed_at: + continue + comment_id = comment.get("id") + ack = acks.get(str(comment_id)) + if ack is not None and ack.get("head_sha") == head_sha: + continue # explicitly triaged for this exact head sha + verdict.late_comments.append( + { + "id": comment_id, + "path": comment.get("path"), + "line": comment.get("line"), + "created_at": created_at, + "body_head": (comment.get("body") or "")[:200], + } + ) if verdict.late_comments: verdict.ok = False verdict.reasons.append( - f"{len(verdict.late_comments)} review comment(s) posted after the head commit " - f"({head_committed_at}) -- read and triage before merging" + f"{len(verdict.late_comments)} unacknowledged review comment(s) posted after the head commit " + f"({head_committed_at}) -- read and `ack` (if not actionable) or fix before merging" ) elif review_comments: verdict.reasons.append("could not determine head commit timestamp; late-comment check skipped") @@ -206,7 +326,9 @@ def _emit(verdict: GateVerdict, as_json: bool) -> None: for reason in verdict.reasons: print(f" - {reason}") for late in verdict.late_comments: - print(f" late comment [{late['path']}:{late['line']}] {late['created_at']}: {late['body_head']}") + print( + f" late comment id={late['id']} [{late['path']}:{late['line']}] {late['created_at']}: {late['body_head']}" + ) def main(argv: list[str] | None = None) -> int: @@ -215,20 +337,33 @@ def main(argv: list[str] | None = None) -> int: record_p = sub.add_parser("record", help="Run local verification against a PR's current head and persist a receipt") record_p.add_argument("pr", type=int) - record_p.add_argument( - "--command", required=True, help="Local verification command to run, e.g. 'devtools verify --quick'" - ) + record_p.add_argument("--command", required=True, help="Local verification command to run, e.g. 'devtools verify'") check_p = sub.add_parser("check", help="Decide whether a PR is safe to merge right now") check_p.add_argument("pr", type=int) check_p.add_argument("--max-age-s", type=int, default=_DEFAULT_MAX_AGE_S) + check_p.add_argument("--poll-rounds", type=int, default=_DEFAULT_POLL_ROUNDS) + check_p.add_argument("--poll-interval-s", type=int, default=_DEFAULT_POLL_INTERVAL_S) check_p.add_argument("--json", action="store_true", dest="as_json") + ack_p = sub.add_parser("ack", help="Acknowledge a specific review comment as triaged for the current head sha") + ack_p.add_argument("pr", type=int) + ack_p.add_argument("comment_id", type=int) + ack_p.add_argument("--reason", required=True, help="Why this comment does not block merging") + args = parser.parse_args(argv) if args.action == "record": return cmd_record(args.pr, args.command) - return cmd_check(args.pr, max_age_s=args.max_age_s, as_json=args.as_json) + if args.action == "ack": + return cmd_ack(args.pr, args.comment_id, reason=args.reason) + return cmd_check( + args.pr, + max_age_s=args.max_age_s, + poll_rounds=args.poll_rounds, + poll_interval_s=args.poll_interval_s, + as_json=args.as_json, + ) if __name__ == "__main__": diff --git a/tests/unit/devtools/test_merge_gate.py b/tests/unit/devtools/test_merge_gate.py index 00a10b28f7..2f1b86a8fe 100644 --- a/tests/unit/devtools/test_merge_gate.py +++ b/tests/unit/devtools/test_merge_gate.py @@ -10,12 +10,30 @@ from devtools import merge_gate -def _fake_run(pr_view: dict[str, object], comments: list[dict[str, object]], local_exit: int = 0) -> object: +def _fake_run( + pr_view: dict[str, object], + comments: list[dict[str, object]], + local_exit: int = 0, + local_head_sha: str = "abc123", + dirty: bool = False, + poll_rounds: list[list[dict[str, object]]] | None = None, +) -> object: + """``comments`` is returned on every poll round unless ``poll_rounds`` gives + an explicit per-round sequence (for testing the multi-round poll itself).""" + comment_rounds: list[list[dict[str, object]]] = poll_rounds if poll_rounds is not None else [comments] + call_count = {"api": 0} + def _run(cmd: list[str], **kwargs: object) -> MagicMock: if cmd[:3] == ["gh", "pr", "view"]: return MagicMock(returncode=0, stdout=json.dumps(pr_view), stderr="") if cmd[:2] == ["gh", "api"]: - return MagicMock(returncode=0, stdout=json.dumps(comments), stderr="") + round_index = min(call_count["api"], len(comment_rounds) - 1) + call_count["api"] += 1 + return MagicMock(returncode=0, stdout=json.dumps(comment_rounds[round_index]), stderr="") + if cmd[:2] == ["git", "rev-parse"]: + return MagicMock(returncode=0, stdout=local_head_sha + "\n", stderr="") + if cmd[:2] == ["git", "status"]: + return MagicMock(returncode=0, stdout=" M dirty.py\n" if dirty else "", stderr="") return MagicMock(returncode=local_exit, stdout="ok\n", stderr="") return _run @@ -26,15 +44,16 @@ def test_record_persists_receipt_keyed_to_current_head_sha(monkeypatch: pytest.M monkeypatch.setattr( subprocess, "run", - _fake_run({"headRefOid": "abc123", "headRefName": "feature/x"}, []), + _fake_run({"headRefOid": "abc123", "headRefName": "feature/x"}, [], local_head_sha="abc123"), ) - exit_code = merge_gate.cmd_record(42, "true") + exit_code = merge_gate.cmd_record(42, "devtools test tests/unit/foo.py") assert exit_code == 0 receipt = json.loads(merge_gate._receipt_path(42).read_text()) assert receipt["head_sha"] == "abc123" assert receipt["exit_code"] == 0 + assert receipt["skips_tests"] is False def test_record_captures_nonzero_local_command_exit(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: @@ -42,16 +61,60 @@ def test_record_captures_nonzero_local_command_exit(monkeypatch: pytest.MonkeyPa monkeypatch.setattr( subprocess, "run", - _fake_run({"headRefOid": "abc123", "headRefName": "feature/x"}, [], local_exit=1), + _fake_run({"headRefOid": "abc123", "headRefName": "feature/x"}, [], local_exit=1, local_head_sha="abc123"), ) - exit_code = merge_gate.cmd_record(42, "false") + exit_code = merge_gate.cmd_record(42, "devtools test somefile") assert exit_code == 1 receipt = json.loads(merge_gate._receipt_path(42).read_text()) assert receipt["exit_code"] == 1 +def test_record_refuses_when_local_checkout_does_not_match_pr_head( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + monkeypatch.chdir(tmp_path) + monkeypatch.setattr( + subprocess, + "run", + _fake_run({"headRefOid": "abc123", "headRefName": "feature/x"}, [], local_head_sha="deadbeef"), + ) + + exit_code = merge_gate.cmd_record(42, "devtools test somefile") + + assert exit_code == 2 + assert not merge_gate._receipt_path(42).exists() + + +def test_record_refuses_when_checkout_is_dirty(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + monkeypatch.chdir(tmp_path) + monkeypatch.setattr( + subprocess, + "run", + _fake_run({"headRefOid": "abc123", "headRefName": "feature/x"}, [], local_head_sha="abc123", dirty=True), + ) + + exit_code = merge_gate.cmd_record(42, "devtools test somefile") + + assert exit_code == 2 + assert not merge_gate._receipt_path(42).exists() + + +def test_record_flags_a_test_skipping_command(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + monkeypatch.chdir(tmp_path) + monkeypatch.setattr( + subprocess, + "run", + _fake_run({"headRefOid": "abc123", "headRefName": "feature/x"}, [], local_head_sha="abc123"), + ) + + merge_gate.cmd_record(42, "devtools verify --quick") + + receipt = json.loads(merge_gate._receipt_path(42).read_text()) + assert receipt["skips_tests"] is True + + def _base_pr_view(head_sha: str = "abc123", committed_date: str = "2026-08-01T12:00:00Z") -> dict[str, object]: return { "headRefOid": head_sha, @@ -62,11 +125,16 @@ def _base_pr_view(head_sha: str = "abc123", committed_date: str = "2026-08-01T12 } +def _record(monkeypatch: pytest.MonkeyPatch, pr_view: dict[str, object], command: str = "devtools test x") -> None: + monkeypatch.setattr(subprocess, "run", _fake_run(pr_view, [], local_head_sha=str(pr_view["headRefOid"]))) + merge_gate.cmd_record(42, command) + + def test_check_blocks_when_no_receipt_exists(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: monkeypatch.chdir(tmp_path) monkeypatch.setattr(subprocess, "run", _fake_run(_base_pr_view(), [])) - exit_code = merge_gate.cmd_check(42, max_age_s=3600, as_json=False) + exit_code = merge_gate.cmd_check(42, max_age_s=3600, poll_rounds=1, poll_interval_s=0, as_json=False) assert exit_code == 1 @@ -75,22 +143,21 @@ def test_check_ok_when_receipt_fresh_and_matches_head_with_no_late_comments( monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: monkeypatch.chdir(tmp_path) - monkeypatch.setattr(subprocess, "run", _fake_run(_base_pr_view(), [])) - merge_gate.cmd_record(42, "true") + _record(monkeypatch, _base_pr_view()) - exit_code = merge_gate.cmd_check(42, max_age_s=3600, as_json=False) + monkeypatch.setattr(subprocess, "run", _fake_run(_base_pr_view(), [])) + exit_code = merge_gate.cmd_check(42, max_age_s=3600, poll_rounds=1, poll_interval_s=0, as_json=False) assert exit_code == 0 def test_check_blocks_when_receipt_is_for_a_stale_sha(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: monkeypatch.chdir(tmp_path) - monkeypatch.setattr(subprocess, "run", _fake_run(_base_pr_view(head_sha="abc123"), [])) - merge_gate.cmd_record(42, "true") + _record(monkeypatch, _base_pr_view(head_sha="abc123")) # A new commit landed after the receipt was recorded. monkeypatch.setattr(subprocess, "run", _fake_run(_base_pr_view(head_sha="def456"), [])) - exit_code = merge_gate.cmd_check(42, max_age_s=3600, as_json=False) + exit_code = merge_gate.cmd_check(42, max_age_s=3600, poll_rounds=1, poll_interval_s=0, as_json=False) assert exit_code == 1 @@ -98,19 +165,41 @@ def test_check_blocks_when_receipt_is_for_a_stale_sha(monkeypatch: pytest.Monkey def test_check_blocks_on_review_comment_newer_than_head_commit(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: monkeypatch.chdir(tmp_path) pr_view = _base_pr_view(committed_date="2026-08-01T12:00:00Z") + _record(monkeypatch, pr_view) + late_comment = [ { + "id": 111, "path": "polylogue/foo.py", "line": 10, "created_at": "2026-08-01T12:05:00Z", "body": "this is a real finding", } ] - monkeypatch.setattr(subprocess, "run", _fake_run(pr_view, [])) - merge_gate.cmd_record(42, "true") - monkeypatch.setattr(subprocess, "run", _fake_run(pr_view, late_comment)) - exit_code = merge_gate.cmd_check(42, max_age_s=3600, as_json=False) + exit_code = merge_gate.cmd_check(42, max_age_s=3600, poll_rounds=1, poll_interval_s=0, as_json=False) + + assert exit_code == 1 + + +def test_check_catches_a_comment_that_arrives_only_on_a_later_poll_round( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """The PR #3502 incident: CodeRabbit posts 30-60s after the first snapshot.""" + monkeypatch.chdir(tmp_path) + pr_view = _base_pr_view(committed_date="2026-08-01T12:00:00Z") + _record(monkeypatch, pr_view) + + late_comment = { + "id": 222, + "path": "polylogue/foo.py", + "line": 10, + "created_at": "2026-08-01T12:05:00Z", + "body": "arrived late", + } + # Round 1: empty. Round 2: the comment has landed. + monkeypatch.setattr(subprocess, "run", _fake_run(pr_view, [], poll_rounds=[[], [late_comment]])) + exit_code = merge_gate.cmd_check(42, max_age_s=3600, poll_rounds=2, poll_interval_s=0, as_json=False) assert exit_code == 1 @@ -118,39 +207,106 @@ def test_check_blocks_on_review_comment_newer_than_head_commit(monkeypatch: pyte def test_check_ignores_comment_older_than_head_commit(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: monkeypatch.chdir(tmp_path) pr_view = _base_pr_view(committed_date="2026-08-01T12:00:00Z") + _record(monkeypatch, pr_view) + stale_comment = [ { + "id": 333, "path": "polylogue/foo.py", "line": 10, "created_at": "2026-08-01T11:55:00Z", "body": "already addressed by the fix commit", } ] + monkeypatch.setattr(subprocess, "run", _fake_run(pr_view, stale_comment)) + exit_code = merge_gate.cmd_check(42, max_age_s=3600, poll_rounds=1, poll_interval_s=0, as_json=False) + + assert exit_code == 0 + + +def test_check_allows_an_acknowledged_late_comment_for_the_same_head_sha( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + monkeypatch.chdir(tmp_path) + pr_view = _base_pr_view(committed_date="2026-08-01T12:00:00Z") + _record(monkeypatch, pr_view) + + late_comment = [ + { + "id": 444, + "path": "polylogue/foo.py", + "line": 10, + "created_at": "2026-08-01T12:05:00Z", + "body": "false positive, already fine", + } + ] monkeypatch.setattr(subprocess, "run", _fake_run(pr_view, [])) - merge_gate.cmd_record(42, "true") + merge_gate.cmd_ack(42, 444, reason="false positive") - monkeypatch.setattr(subprocess, "run", _fake_run(pr_view, stale_comment)) - exit_code = merge_gate.cmd_check(42, max_age_s=3600, as_json=False) + monkeypatch.setattr(subprocess, "run", _fake_run(pr_view, late_comment)) + exit_code = merge_gate.cmd_check(42, max_age_s=3600, poll_rounds=1, poll_interval_s=0, as_json=False) assert exit_code == 0 +def test_check_ignores_an_ack_recorded_for_a_different_head_sha( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """A new push must invalidate old acks -- otherwise a stale triage silently covers new code.""" + monkeypatch.chdir(tmp_path) + old_pr_view = _base_pr_view(head_sha="abc123", committed_date="2026-08-01T12:00:00Z") + monkeypatch.setattr(subprocess, "run", _fake_run(old_pr_view, [])) + merge_gate.cmd_ack(42, 555, reason="false positive on the old commit") + + new_pr_view = _base_pr_view(head_sha="def456", committed_date="2026-08-01T13:00:00Z") + _record(monkeypatch, new_pr_view) + late_comment = [ + { + "id": 555, + "path": "polylogue/foo.py", + "line": 10, + "created_at": "2026-08-01T13:05:00Z", + "body": "same comment id, but this is a new push", + } + ] + monkeypatch.setattr(subprocess, "run", _fake_run(new_pr_view, late_comment)) + exit_code = merge_gate.cmd_check(42, max_age_s=3600, poll_rounds=1, poll_interval_s=0, as_json=False) + + assert exit_code == 1 + + def test_check_blocks_when_pr_is_not_open(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: monkeypatch.chdir(tmp_path) pr_view = _base_pr_view() pr_view["state"] = "MERGED" monkeypatch.setattr(subprocess, "run", _fake_run(pr_view, [])) - exit_code = merge_gate.cmd_check(42, max_age_s=3600, as_json=False) + exit_code = merge_gate.cmd_check(42, max_age_s=3600, poll_rounds=1, poll_interval_s=0, as_json=False) assert exit_code == 1 def test_check_blocks_when_receipt_older_than_max_age(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: monkeypatch.chdir(tmp_path) - monkeypatch.setattr(subprocess, "run", _fake_run(_base_pr_view(), [])) - merge_gate.cmd_record(42, "true") + _record(monkeypatch, _base_pr_view()) - exit_code = merge_gate.cmd_check(42, max_age_s=-1, as_json=False) + monkeypatch.setattr(subprocess, "run", _fake_run(_base_pr_view(), [])) + exit_code = merge_gate.cmd_check(42, max_age_s=-1, poll_rounds=1, poll_interval_s=0, as_json=False) assert exit_code == 1 + + +def test_check_reports_advisory_when_receipt_command_skips_tests( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + monkeypatch.chdir(tmp_path) + pr_view = _base_pr_view() + _record(monkeypatch, pr_view, command="devtools verify --quick") + + monkeypatch.setattr(subprocess, "run", _fake_run(pr_view, [])) + exit_code = merge_gate.cmd_check(42, max_age_s=3600, poll_rounds=1, poll_interval_s=0, as_json=False) + + # Advisory only -- does not block by itself, but is reported. + assert exit_code == 0 + receipt = json.loads(merge_gate._receipt_path(42).read_text()) + assert receipt["skips_tests"] is True From bb3cbbd2b3b9d0faf95e36056ea03c3df7a7522a Mon Sep 17 00:00:00 2001 From: Sinity Date: Sat, 1 Aug 2026 16:07:24 +0200 Subject: [PATCH 3/3] fix(devtools): address CodeRabbit's second review pass on merge-gate Real findings, all fixed: - use_when text overclaimed that `record` checks out the PR's head commit; it only verifies the current checkout already matches. - `_command_skips_tests` flagged the documented happy-path command itself (`devtools verify`) as test-skipping -- added it to the positive markers. - An empty --command or a missing executable raised an unhandled IndexError/FileNotFoundError instead of a clean refusal. - `gh api .../comments` without pagination silently hid comments past the first page (30-100 items) from the late-comment check. - The late-comment check only watched inline diff comments, missing issue-level PR comments and review summary bodies -- now merges and normalizes all three, dropping empty-bodied entries (e.g. a plain APPROVE review). - `commits[-1]` assumed positional ordering matched the PR head; switched to matching by oid, since a capped/reordered commits array could silently mispoint the late-comment timestamp. - Receipt/ack JSON reads had no guard against a truncated or corrupt file; added a shared `_read_json_object` used everywhere, and `cmd_ack` now refuses explicitly on a corrupt ack file rather than silently discarding prior acknowledgements. - Critical: when the head commit's timestamp couldn't be determined, the late-comment check silently reported OK (an `elif` that never set ok=False) -- exactly the "fails closed" contract this tool exists to guarantee. Now blocks explicitly in that case. Verification: devtools test tests/unit/devtools/test_merge_gate.py -- 20 passed (added: nonzero-receipt-exit-code block, dirty mergeStateStatus block, comment-polling-failure block, a late review body caught alongside inline comments, frozen_clock for the freshness test per repo convention). devtools verify --quick exit 0 (one transient unrelated SQLite disk-I/O error in demo-corpus- datasheet rendering reproduced as a one-off and cleared on rerun). --- devtools/command_catalog.py | 5 +- devtools/merge_gate.py | 118 ++++++++++++++++++++++--- tests/unit/devtools/test_merge_gate.py | 99 +++++++++++++++++++-- 3 files changed, 198 insertions(+), 24 deletions(-) diff --git a/devtools/command_catalog.py b/devtools/command_catalog.py index eda3bba122..5c18691f95 100644 --- a/devtools/command_catalog.py +++ b/devtools/command_catalog.py @@ -687,8 +687,9 @@ def to_dict(self) -> dict[str, object]: use_when=( "Immediately before squash-merging any PR in a merge train, replacing coordinator memory " "(grace-period comment polling, remembering to run the broader local test suite CI skips " - 'per-PR) with a check that fails closed. `record --command "..."` checks out the ' - "PR's exact head commit (refuses on any mismatch or dirty tree), runs a local verification " + 'per-PR) with a check that fails closed. `record --command "..."` requires the current ' + "checkout to already be the PR's exact head commit with a clean tree (it refuses otherwise), " + "runs a local verification " "command, and persists a receipt flagging commands that look like they skip tests (e.g. " "`verify --quick`). `check ` polls review comments across a real grace window (default " "3x20s, covering CodeRabbit's 30-60s late-arrival window) and BLOCKs unless a receipt exists " diff --git a/devtools/merge_gate.py b/devtools/merge_gate.py index 18e89db250..7ab0e0aa41 100644 --- a/devtools/merge_gate.py +++ b/devtools/merge_gate.py @@ -74,7 +74,7 @@ # flagged as an advisory, since the whole point is not trusting a plausible- # looking command string without comment. _TEST_SKIPPING_MARKERS: tuple[str, ...] = ("verify --quick", "verify --lab") -_LOOKS_LIKE_TESTS_MARKERS: tuple[str, ...] = ("test", "pytest", "verify --all") +_LOOKS_LIKE_TESTS_MARKERS: tuple[str, ...] = ("test", "pytest", "verify --all", "devtools verify") def _gh_json(args: list[str]) -> Any: @@ -84,6 +84,38 @@ def _gh_json(args: list[str]) -> Any: return json.loads(result.stdout) +def _gh_json_paginated(args: list[str]) -> list[Any]: + """Like ``_gh_json`` but follows pagination -- the GitHub REST list + endpoints cap at 30-100 items per page, and a PR with more review comments + than that would otherwise silently hide later (possibly late-arriving) + ones from the late-comment check. ``--slurp`` wraps every page's own JSON + array into one outer array, which this then flattens.""" + result = subprocess.run(["gh", *args, "--paginate", "--slurp"], capture_output=True, text=True, timeout=120) + if result.returncode != 0: + raise RuntimeError(result.stderr.strip()[:300] or f"gh {' '.join(args)} --paginate failed") + pages = json.loads(result.stdout) + items: list[Any] = [] + for page in pages: + if isinstance(page, list): + items.extend(page) + else: + items.append(page) + return items + + +def _read_json_object(path: Path) -> dict[str, Any] | None: + """Return the parsed object, or None when the file is absent, unreadable, + truncated, or not a JSON object (a hand edit or an interrupted write must + not raise a traceback out of a merge-safety check).""" + if not path.exists(): + return None + try: + loaded = json.loads(path.read_text()) + except (OSError, json.JSONDecodeError): + return None + return loaded if isinstance(loaded, dict) else None + + def _git_head_sha() -> str | None: result = subprocess.run(["git", "rev-parse", "HEAD"], capture_output=True, text=True, timeout=15) if result.returncode != 0: @@ -144,8 +176,15 @@ def cmd_record(pr: int, command: str) -> int: return 2 argv = shlex.split(command) + if not argv: + print("REFUSING to record: --command is empty after shell splitting.", file=sys.stderr) + return 2 started = time.time() - result = subprocess.run(argv, capture_output=True, text=True) + try: + result = subprocess.run(argv, capture_output=True, text=True) + except OSError as exc: + print(f"REFUSING to record: could not run {command!r}: {exc}", file=sys.stderr) + return 2 duration_s = round(time.time() - started, 2) receipt = { @@ -180,7 +219,17 @@ def cmd_ack(pr: int, comment_id: int, *, reason: str) -> int: head_sha = info["headRefOid"] ack_path = _ack_path(pr) - acks: dict[str, Any] = json.loads(ack_path.read_text()) if ack_path.exists() else {} + if ack_path.exists(): + acks = _read_json_object(ack_path) + if acks is None: + print( + f"REFUSING to ack: {ack_path} exists but is unreadable/corrupt -- fix or remove it by hand " + "first, rather than silently losing prior acknowledgements.", + file=sys.stderr, + ) + return 2 + else: + acks = {} acks[str(comment_id)] = {"head_sha": head_sha, "reason": reason, "acked_at": time.time()} _RECEIPT_DIR.mkdir(parents=True, exist_ok=True) ack_path.write_text(json.dumps(acks, indent=2)) @@ -189,11 +238,51 @@ def cmd_ack(pr: int, comment_id: int, *, reason: str) -> int: def _fetch_review_comments(pr: int) -> list[dict[str, Any]] | None: + """Combine every top-level review signal the late-comment check should + see: inline diff comments, issue-level PR comments, and review bodies + (a review's own summary text is a separate object from its line + comments -- see GitHub's REST API docs). All three are normalized to a + common shape (id, created_at, path, line, body) and empty-bodied entries + (e.g. an APPROVE review with no summary text) are dropped -- they carry + no signal for triage.""" + normalized: list[dict[str, Any]] = [] try: - result: list[dict[str, Any]] = _gh_json(["api", f"repos/{{owner}}/{{repo}}/pulls/{pr}/comments"]) + inline = _gh_json_paginated(["api", f"repos/{{owner}}/{{repo}}/pulls/{pr}/comments"]) + for item in inline: + normalized.append( + { + "id": item.get("id"), + "created_at": item.get("created_at", ""), + "path": item.get("path"), + "line": item.get("line"), + "body": item.get("body") or "", + } + ) + issue_comments = _gh_json_paginated(["api", f"repos/{{owner}}/{{repo}}/issues/{pr}/comments"]) + for item in issue_comments: + normalized.append( + { + "id": item.get("id"), + "created_at": item.get("created_at", ""), + "path": None, + "line": None, + "body": item.get("body") or "", + } + ) + reviews = _gh_json_paginated(["api", f"repos/{{owner}}/{{repo}}/pulls/{pr}/reviews"]) + for item in reviews: + normalized.append( + { + "id": item.get("id"), + "created_at": item.get("submitted_at", ""), + "path": None, + "line": None, + "body": item.get("body") or "", + } + ) except (RuntimeError, json.JSONDecodeError, OSError, subprocess.SubprocessError): return None - return result + return [comment for comment in normalized if comment["body"].strip()] def _poll_stable_comments(pr: int, *, rounds: int, interval_s: int) -> list[dict[str, Any]] | None: @@ -245,18 +334,18 @@ def cmd_check(pr: int, *, max_age_s: int, poll_rounds: int, poll_interval_s: int verdict.reasons.append(f"mergeStateStatus is {mss!r} (expected CLEAN/UNSTABLE/UNKNOWN)") commits = info.get("commits") or [] - head_commit = commits[-1] if commits else None + head_commit = next((commit for commit in commits if commit.get("oid") == head_sha), None) head_committed_at = head_commit.get("committedDate") if head_commit else None receipt_path = _receipt_path(pr) - if not receipt_path.exists(): + receipt = _read_json_object(receipt_path) + if receipt is None: verdict.ok = False verdict.reasons.append( - f"no local verification receipt found at {receipt_path} -- run " + f"no local verification receipt found (or it is unreadable) at {receipt_path} -- run " f'`devtools workspace merge-gate record {pr} --command "..."` against the current head first' ) else: - receipt = json.loads(receipt_path.read_text()) verdict.receipt = receipt if receipt.get("head_sha") != head_sha: verdict.ok = False @@ -284,8 +373,7 @@ def cmd_check(pr: int, *, max_age_s: int, poll_rounds: int, poll_interval_s: int verdict.reasons.append("could not fetch review comments after polling") review_comments = [] - ack_path = _ack_path(pr) - acks: dict[str, Any] = json.loads(ack_path.read_text()) if ack_path.exists() else {} + acks = _read_json_object(_ack_path(pr)) or {} if head_committed_at: for comment in review_comments: @@ -311,8 +399,12 @@ def cmd_check(pr: int, *, max_age_s: int, poll_rounds: int, poll_interval_s: int f"{len(verdict.late_comments)} unacknowledged review comment(s) posted after the head commit " f"({head_committed_at}) -- read and `ack` (if not actionable) or fix before merging" ) - elif review_comments: - verdict.reasons.append("could not determine head commit timestamp; late-comment check skipped") + else: + verdict.ok = False + verdict.reasons.append( + "could not determine the head commit timestamp, so the late-comment check cannot run -- " + "refusing to report OK" + ) _emit(verdict, as_json) return 0 if verdict.ok else 1 diff --git a/tests/unit/devtools/test_merge_gate.py b/tests/unit/devtools/test_merge_gate.py index 2f1b86a8fe..ee6fc77e9e 100644 --- a/tests/unit/devtools/test_merge_gate.py +++ b/tests/unit/devtools/test_merge_gate.py @@ -8,6 +8,7 @@ import pytest from devtools import merge_gate +from tests.infra.frozen_clock import FrozenClock def _fake_run( @@ -18,18 +19,25 @@ def _fake_run( dirty: bool = False, poll_rounds: list[list[dict[str, object]]] | None = None, ) -> object: - """``comments`` is returned on every poll round unless ``poll_rounds`` gives - an explicit per-round sequence (for testing the multi-round poll itself).""" + """``comments`` (inline review comments) is returned on every poll round + unless ``poll_rounds`` gives an explicit per-round sequence (for testing + the multi-round poll itself). Issue comments and review bodies are always + empty here -- covered separately in the normalization tests below.""" comment_rounds: list[list[dict[str, object]]] = poll_rounds if poll_rounds is not None else [comments] - call_count = {"api": 0} + call_count = {"round": 0} def _run(cmd: list[str], **kwargs: object) -> MagicMock: + joined = " ".join(cmd) if cmd[:3] == ["gh", "pr", "view"]: return MagicMock(returncode=0, stdout=json.dumps(pr_view), stderr="") - if cmd[:2] == ["gh", "api"]: - round_index = min(call_count["api"], len(comment_rounds) - 1) - call_count["api"] += 1 - return MagicMock(returncode=0, stdout=json.dumps(comment_rounds[round_index]), stderr="") + if "/issues/" in joined and "/comments" in joined: + return MagicMock(returncode=0, stdout=json.dumps([[]]), stderr="") + if "/pulls/" in joined and "/reviews" in joined: + return MagicMock(returncode=0, stdout=json.dumps([[]]), stderr="") + if "/pulls/" in joined and "/comments" in joined: + round_index = min(call_count["round"], len(comment_rounds) - 1) + call_count["round"] += 1 + return MagicMock(returncode=0, stdout=json.dumps([comment_rounds[round_index]]), stderr="") if cmd[:2] == ["git", "rev-parse"]: return MagicMock(returncode=0, stdout=local_head_sha + "\n", stderr="") if cmd[:2] == ["git", "status"]: @@ -286,12 +294,85 @@ def test_check_blocks_when_pr_is_not_open(monkeypatch: pytest.MonkeyPatch, tmp_p assert exit_code == 1 -def test_check_blocks_when_receipt_older_than_max_age(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: +def test_check_blocks_when_receipt_older_than_max_age( + frozen_clock: FrozenClock, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: monkeypatch.chdir(tmp_path) _record(monkeypatch, _base_pr_view()) + frozen_clock.advance(7200) monkeypatch.setattr(subprocess, "run", _fake_run(_base_pr_view(), [])) - exit_code = merge_gate.cmd_check(42, max_age_s=-1, poll_rounds=1, poll_interval_s=0, as_json=False) + exit_code = merge_gate.cmd_check(42, max_age_s=3600, poll_rounds=1, poll_interval_s=0, as_json=False) + + assert exit_code == 1 + + +def test_check_blocks_when_receipt_exit_code_is_nonzero(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + monkeypatch.chdir(tmp_path) + pr_view = _base_pr_view() + monkeypatch.setattr(subprocess, "run", _fake_run(pr_view, [], local_exit=1, local_head_sha="abc123")) + merge_gate.cmd_record(42, "devtools test x") + + monkeypatch.setattr(subprocess, "run", _fake_run(pr_view, [])) + exit_code = merge_gate.cmd_check(42, max_age_s=3600, poll_rounds=1, poll_interval_s=0, as_json=False) + + assert exit_code == 1 + + +def test_check_blocks_when_merge_state_status_is_dirty(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + monkeypatch.chdir(tmp_path) + pr_view = _base_pr_view() + _record(monkeypatch, pr_view) + + pr_view["mergeStateStatus"] = "DIRTY" + monkeypatch.setattr(subprocess, "run", _fake_run(pr_view, [])) + exit_code = merge_gate.cmd_check(42, max_age_s=3600, poll_rounds=1, poll_interval_s=0, as_json=False) + + assert exit_code == 1 + + +def test_check_blocks_when_comment_polling_fails(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + monkeypatch.chdir(tmp_path) + pr_view = _base_pr_view() + _record(monkeypatch, pr_view) + + def _run_with_broken_api(cmd: list[str], **kwargs: object) -> MagicMock: + if cmd[:3] == ["gh", "pr", "view"]: + return MagicMock(returncode=0, stdout=json.dumps(pr_view), stderr="") + if cmd[:2] == ["gh", "api"]: + return MagicMock(returncode=1, stdout="", stderr="rate limited") + return MagicMock(returncode=0, stdout="", stderr="") + + monkeypatch.setattr(subprocess, "run", _run_with_broken_api) + exit_code = merge_gate.cmd_check(42, max_age_s=3600, poll_rounds=1, poll_interval_s=0, as_json=False) + + assert exit_code == 1 + + +def test_check_catches_a_late_review_body_not_just_inline_comments( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """Review summaries and issue-level comments carry findings too -- a gate + that only watches inline diff comments misses them.""" + monkeypatch.chdir(tmp_path) + pr_view = _base_pr_view(committed_date="2026-08-01T12:00:00Z") + _record(monkeypatch, pr_view) + + def _run_with_late_review(cmd: list[str], **kwargs: object) -> MagicMock: + joined = " ".join(cmd) + if cmd[:3] == ["gh", "pr", "view"]: + return MagicMock(returncode=0, stdout=json.dumps(pr_view), stderr="") + if "/pulls/" in joined and "/reviews" in joined: + late_review = [{"id": 999, "submitted_at": "2026-08-01T12:10:00Z", "body": "Request changes: real bug"}] + return MagicMock(returncode=0, stdout=json.dumps([late_review]), stderr="") + if "/issues/" in joined and "/comments" in joined: + return MagicMock(returncode=0, stdout=json.dumps([[]]), stderr="") + if "/pulls/" in joined and "/comments" in joined: + return MagicMock(returncode=0, stdout=json.dumps([[]]), stderr="") + return MagicMock(returncode=0, stdout="", stderr="") + + monkeypatch.setattr(subprocess, "run", _run_with_late_review) + exit_code = merge_gate.cmd_check(42, max_age_s=3600, poll_rounds=1, poll_interval_s=0, as_json=False) assert exit_code == 1