diff --git a/.github/PULL_REQUEST_TEMPLATE/pr_default.md b/.github/PULL_REQUEST_TEMPLATE/pr_default.md index 1ac5f82d05..933c146a84 100644 --- a/.github/PULL_REQUEST_TEMPLATE/pr_default.md +++ b/.github/PULL_REQUEST_TEMPLATE/pr_default.md @@ -4,8 +4,12 @@ Describe what changed and the user or maintainer value in one or two sentences. ## Linked Issues - + - Closes # +- Refs # - Relates to # ## Stack / Dependency diff --git a/docs/dev_guide_reference.md b/docs/dev_guide_reference.md index 7e31ee558c..6d789dff86 100644 --- a/docs/dev_guide_reference.md +++ b/docs/dev_guide_reference.md @@ -732,8 +732,9 @@ before the queue auto-merges a PR: - **Audit record**: the job emits a `merge_queue_gate.v1` audit with the evaluated head SHA, the source-head SHA encoded in the queue ref and its binding verdict, queue merging strategy, base SHA, label set, metadata digest and metadata-verdict status, gate-verdict status, staleness - verdict, CI conclusion, and reviewer-thread resolution plus requested-reviewer status, so every - merge decision is inspectable and reproducible. + verdict, CI conclusion, reviewer-thread resolution plus requested-reviewer status, and the + current closing-discipline status/blockers from PR commit and issue metadata, so every merge + decision is inspectable and reproducible. - **Self-test**: `uv run python scripts/dev/merge_queue_gate.py --self-test` exercises the fail-closed contract deterministically (the issue #6274 validation scenarios). @@ -2594,6 +2595,14 @@ workflow uses the existing `main_ci_incident_reconcile.py` signal and requires two newer consecutive decisive green runs before it posts an evidence comment and closes an incident as completed. Active, pending, malformed, or concurrent-change cases remain open. +Cancelled or superseded runs are neutral: they count as neither green nor red +and cannot satisfy either slot in the two-green streak. + +To preserve that evidence boundary, pull requests must use `Refs #N` for these +incidents instead of GitHub's semantic closing keywords (`Closes`, `Fixes`, or +`Resolves`). The blocking PR Contract Check rejects semantic closure for either +the canonical body marker or its compatibility label, leaving the scheduled +reconciler as the sole closer after the two-green criterion is met. The Actions run evidence window is paginated. The reconciler reads full workflow-run pages and stops only after two decisive completed green/red runs diff --git a/scripts/ci/pr_contract_check.py b/scripts/ci/pr_contract_check.py index 468be36861..5fb74a9e90 100755 --- a/scripts/ci/pr_contract_check.py +++ b/scripts/ci/pr_contract_check.py @@ -35,11 +35,21 @@ ) from scripts.dev.gh_pr_label_rest import add_label # noqa: E402 -# Match keywords followed by #N or a GitHub issue URL +# Match GitHub closing keywords followed by a local/cross-repository issue reference or URL. CLOSING_PATTERN = re.compile( - r"\b(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?)\s+`?(?:#(\d+)|https?://github\.com/[^/\s]+/[^/\s]+/issues/(\d+))\b", + r"\b(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?)\s*:?\s*`?" + r"(?:(?P[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+)#(?P\d+)" + r"|#(?P\d+)" + r"|https?://github\.com/(?P[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+)/issues/" + r"(?P\d+))\b", re.IGNORECASE, ) +CLOSES_DISCIPLINE_TAG = "[closes-discipline]" +MAIN_CI_INCIDENT_MARKER = "ll7-main-red-incident:v1" +MAIN_CI_INCIDENT_LABEL = MAIN_CI_INCIDENT_MARKER +MAIN_CI_INCIDENT_MARKER_RE = re.compile( + rf"(?im)^\s*\s*$" +) EVIDENCE_PATH_PREFIX = "docs/context/evidence/" EVIDENCE_REVIEW_SIDECAR_SUFFIX = ".review.json" EVIDENCE_REVIEW_SCHEMA_VERSION = "evidence-review-marker.v1" @@ -80,15 +90,22 @@ def is_negated(text: str, match_start: int) -> bool: return False +def _find_closed_references(text: str) -> list[tuple[str | None, str]]: + """Extract closing references as ``(target_repo, issue_number)`` pairs.""" + references: list[tuple[str | None, str]] = [] + for match in CLOSING_PATTERN.finditer(text): + if is_negated(text, match.start()): + continue + target_repo = match.group("qualified_repo") or match.group("url_repo") + issue = match.group("qualified_issue") or match.group("issue") or match.group("url_issue") + if issue: + references.append((target_repo, issue)) + return references + + def find_closed_issues(body: str) -> list[str]: """Extract issue numbers that this PR claims to close.""" - issues = [] - for match in CLOSING_PATTERN.finditer(body): - if is_negated(body, match.start()): - continue - num1, num2 = match.groups() - issues.append(num1 or num2) - return sorted({i for i in issues if i}, key=int) + return sorted({issue for _, issue in _find_closed_references(body)}, key=int) def find_title_issues(title: str) -> list[str]: @@ -100,28 +117,89 @@ def find_title_issues(title: str) -> list[str]: def has_declaration_for_issue(issue: str, body: str) -> bool: """Check if there is a closes or refs declaration for the given issue.""" pattern = re.compile( - rf"\b(?:closes?|fixes?|resolves?|refs?|references?)\s+`?(?:#|https?://github\.com/[^/\s]+/[^/\s]+/issues/)?{issue}\b", + rf"\b(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?|refs?|references?)\s*:?\s*`?" + rf"(?:[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+#|" + rf"https?://github\.com/[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+/issues/|#)" + rf"{re.escape(issue)}\b", re.IGNORECASE, ) return bool(pattern.search(body)) -def get_issue_labels(issue: str, repo: str) -> list[str]: - """Query GitHub API to get labels for a specific issue.""" +def get_issue_metadata(issue: str, repo: str) -> tuple[list[str], str] | None: + """Query GitHub for the labels and body needed to classify a closing target. + + ``None`` means that the issue could not be read or validated. Callers that + enforce a closing contract must treat that result as unknown and fail closed. + """ try: res = subprocess.run( - ["gh", "issue", "view", issue, "--json", "labels", "--repo", repo], + ["gh", "issue", "view", issue, "--json", "labels,body", "--repo", repo], capture_output=True, text=True, timeout=10, check=False, ) - if res.returncode == 0: - data = json.loads(res.stdout) - return [lbl["name"].lower() for lbl in data.get("labels", [])] + if res.returncode != 0: + return None + data = json.loads(res.stdout) + if not isinstance(data, dict): + return None + if "labels" not in data or "body" not in data: + return None + raw_labels = data["labels"] + if not isinstance(raw_labels, list): + return None + labels: list[str] = [] + for label in raw_labels: + if not isinstance(label, dict) or not isinstance(label.get("name"), str): + return None + labels.append(label["name"].lower()) + raw_body = data["body"] + if raw_body is None: + body = "" + elif isinstance(raw_body, str): + body = raw_body + else: + return None + return labels, body except _BEST_EFFORT_ERRORS: - pass - return [] + return None + + +def get_issue_labels(issue: str, repo: str) -> list[str]: + """Query GitHub API to get labels for a specific issue.""" + metadata = get_issue_metadata(issue, repo) + return metadata[0] if metadata is not None else [] + + +def get_pr_commit_messages(pr_number: str, repo: str) -> str | None: + """Return non-empty commit messages for a PR, or ``None`` when unavailable.""" + try: + result = subprocess.run( + [ + "gh", + "api", + "--paginate", + f"repos/{repo}/pulls/{pr_number}/commits?per_page=100", + "--jq", + ".[] | .commit.message", + ], + capture_output=True, + text=True, + timeout=30, + check=False, + ) + if result.returncode != 0: + return None + return result.stdout if result.stdout.strip() else None + except _BEST_EFFORT_ERRORS: + return None + + +def is_main_ci_incident_issue(labels: list[str], body: str) -> bool: + """Return whether issue metadata identifies a canonical main-CI incident.""" + return MAIN_CI_INCIDENT_LABEL in labels or MAIN_CI_INCIDENT_MARKER_RE.search(body) is not None def base_ref_is_resolvable(base_ref: str) -> bool: @@ -205,17 +283,62 @@ def is_file_new(path: str, base_ref: str = "origin/main") -> bool: return res.returncode != 0 -def check_closes_discipline(body: str, repo: str) -> list[str]: - """Rule 1: Demand Refs #N instead of Closes #N if N is an epic issue.""" - blockers = [] - closed_issues = find_closed_issues(body) - for issue in closed_issues: - labels = get_issue_labels(issue, repo) - if "epic" in labels: +def check_closes_discipline( + body: str, + repo: str, + *, + commit_messages: str | None = None, + commit_messages_checked: bool = False, +) -> list[str]: + """Rule 1: protect epic and canonical main-CI incident issue lifecycles. + + When ``commit_messages_checked`` is true, the commit-message source is authoritative for the + PR and an unavailable response fails closed. The default keeps direct/local callers body-only. + """ + blockers: list[str] = [] + sources = [("PR body", body)] + if commit_messages_checked: + if not isinstance(commit_messages, str) or not commit_messages.strip(): blockers.append( - f"BLOCKER: PR body attempts to close epic issue #{issue}. " - f"Epic issues cannot be closed by a single PR. Please use 'Refs #{issue}' instead." + f"BLOCKER: {CLOSES_DISCIPLINE_TAG} Could not verify PR commit messages before " + "evaluating semantic closing references. The check fails closed; retry when " + "GitHub commit metadata is available." ) + else: + sources.append(("PR commit message", commit_messages)) + + local_repo = repo.strip().lower() + seen_issues: set[str] = set() + for source_name, source_text in sources: + for target_repo, issue in _find_closed_references(source_text): + if target_repo is not None and target_repo.lower() != local_repo: + continue + if issue in seen_issues: + continue + seen_issues.add(issue) + metadata = get_issue_metadata(issue, repo) + if metadata is None: + blockers.append( + f"BLOCKER: {CLOSES_DISCIPLINE_TAG} {source_name} could not verify issue " + f"#{issue} metadata before evaluating a semantic closing reference. The " + f"check fails closed; retry when GitHub issue metadata is available." + ) + continue + + labels, issue_body = metadata + if is_main_ci_incident_issue(labels, issue_body): + blockers.append( + f"BLOCKER: {CLOSES_DISCIPLINE_TAG} {source_name} attempts to semantically " + f"close canonical main continuous-integration (CI) incident issue #{issue}. " + f"Use 'Refs #{issue}' instead; the scheduled reconciler owns closure after " + f"two consecutive decisive green runs." + ) + elif "epic" in labels: + blockers.append( + f"BLOCKER: {CLOSES_DISCIPLINE_TAG} {source_name} attempts to close epic issue " + f"#{issue}. Epic issues cannot be closed by a single PR. Please use 'Refs " + f"#{issue}' instead." + ) return blockers @@ -844,7 +967,17 @@ def run_all_checks( infos = [] # 1. Closes-discipline - closes_blockers = check_closes_discipline(body, repo) + commit_messages = None + commit_messages_checked = False + if pr_number: + commit_messages = get_pr_commit_messages(pr_number, repo) + commit_messages_checked = True + closes_blockers = check_closes_discipline( + body, + repo, + commit_messages=commit_messages, + commit_messages_checked=commit_messages_checked, + ) blockers.extend(closes_blockers) # 2. Closure declaration @@ -888,7 +1021,7 @@ def get_status_str(has_failures: bool, is_blocker: bool = True) -> str: return "✅ PASSED" rows.append( - f"| 1. Closes-discipline | {get_status_str(any('closes epic' in b.lower() for b in blockers))} | Demand Refs #N for epic issues |" + f"| 1. Closes-discipline | {get_status_str(any(CLOSES_DISCIPLINE_TAG in b.lower() for b in blockers))} | Demand Refs #N for epic issues and main-CI incidents |" ) rows.append( f"| 2. Closure declaration | {get_status_str(bool(warnings), is_blocker=False)} | Require Closes/Refs for title issues |" diff --git a/scripts/dev/merge_queue_gate.py b/scripts/dev/merge_queue_gate.py index 3d14b02593..f495ef18ea 100644 --- a/scripts/dev/merge_queue_gate.py +++ b/scripts/dev/merge_queue_gate.py @@ -16,6 +16,7 @@ a proven docs-only changed-file set covered by CI's ``paths-ignore`` rules, - no unresolved actionable review threads, - no outstanding explicitly requested reviewers, + - a current closing-discipline recheck over the PR body and commit messages, - the merge queue's ``ALLGREEN`` strategy, so every constituent entry must pass its own required gate check, - staleness-free base (fresh by construction inside the merge queue, where the @@ -30,8 +31,8 @@ It emits a ``merge_queue_gate.v1`` audit record with the evaluated head SHA, queue merging strategy, base SHA, label set, metadata digest and trailer statuses, exact-head changed-coverage status, staleness verdict, CI conclusion, -reviewer-thread resolution, and requested-reviewer status so the merge decision -is inspectable and reproducible. +reviewer-thread resolution, requested-reviewer status, and closing-discipline +status so the merge decision is inspectable and reproducible. The pure function ``evaluate_merge_gate`` is deterministic and exercised by ``--self-test`` (the validation contract for issue #6274). The CLI resolves a @@ -73,6 +74,10 @@ if str(_REPO_ROOT) not in sys.path: sys.path.insert(0, str(_REPO_ROOT)) +from scripts.ci.pr_contract_check import ( # noqa: E402 + check_closes_discipline, + get_pr_commit_messages, +) from scripts.dev.check_pr_ci_status import ( # noqa: E402 _enrich_rest_check_runs, _latest_check_runs, @@ -181,6 +186,8 @@ class MergeGateAudit: passed: bool body_narrative_status: str = "clean" body_not_ready_sentinels: list[str] = field(default_factory=list) + closing_discipline_status: str = "not_evaluated" + closing_discipline_blockers: list[str] = field(default_factory=list) reasons: list[str] = field(default_factory=list) def to_dict(self) -> dict[str, Any]: @@ -271,6 +278,30 @@ def _reviewer_request_reason(status: str) -> str | None: ) +def _closing_discipline_state(pr: dict[str, Any]) -> tuple[str, list[str]]: + """Validate the live closing-discipline result carried by a PR snapshot. + + Older pure-evaluator fixtures do not carry this optional field and retain the + ``not_evaluated`` status. A live snapshot must carry one of the explicit + results so a missing or malformed merge-time recheck cannot be mistaken for a + successful check. + """ + value = pr.get("closing_discipline") + if value is None: + return "not_evaluated", [] + if not isinstance(value, dict): + return "unknown", [] + status = value.get("status") + blockers = value.get("blockers", []) + if status not in {"passed", "blocked", "unavailable"}: + return "unknown", [] + if not isinstance(blockers, list) or not all(isinstance(item, str) for item in blockers): + return "unknown", [] + if status == "passed" and blockers: + return "unknown", blockers + return status, blockers + + def _core_preflight_reasons( *, draft: bool, @@ -319,6 +350,7 @@ def _fail_closed_reasons( # noqa: PLR0913 merge_group_head_binding: str, body_not_ready_sentinels: list[str] | None = None, ancestry_state: str = "", + closing_discipline_status: str = "not_evaluated", ) -> list[str]: """Collect fail-closed reasons for one gate evaluation. @@ -353,6 +385,14 @@ def _fail_closed_reasons( # noqa: PLR0913 if ancestry_state and ancestry_state != "clean": reasons.append("stacked_ancestry_not_independently_mergeable") + + if closing_discipline_status not in {"passed", "not_evaluated"}: + reasons.append( + { + "blocked": "closing_discipline_blocked", + "unavailable": "closing_discipline_unavailable", + }.get(closing_discipline_status, "closing_discipline_not_verified") + ) return reasons @@ -456,7 +496,8 @@ def evaluate_merge_gate( # noqa: C901, PLR0913 - explicit fail-closed admission ``has_current_accepted_gate_verdict`` (``gate_verdict`` / ``gate_verdicts`` / ``comments`` / ``reviews`` body excerpts), ``metadata_digest`` and trusted ``metadata_verdicts``, and - ``reviewers_requested`` when supplied by the live snapshot. + ``reviewers_requested`` when supplied by the live snapshot, plus the + optional live ``closing_discipline`` result. main_sha: current ``main`` HEAD SHA. When both ``base_sha`` and ``main_sha`` are present and differ, the gate fails closed as stale. When either is absent, staleness is reported as ``not_applicable`` (the merge @@ -494,6 +535,7 @@ def evaluate_merge_gate( # noqa: C901, PLR0913 - explicit fail-closed admission body_text = str(pr.get("body") or "") body_not_ready_sentinels = find_not_ready_body_sentinels(body_text) body_narrative_status = _resolve_narrative_status(body_text, body_not_ready_sentinels) + closing_discipline_status, closing_discipline_blockers = _closing_discipline_state(pr) if ci_overall is None: ci_overall = str((pr.get("checks") or {}).get("overall", "") or "") @@ -559,6 +601,7 @@ def evaluate_merge_gate( # noqa: C901, PLR0913 - explicit fail-closed admission merge_group_head_binding=merge_group_head_binding, body_not_ready_sentinels=body_not_ready_sentinels, ancestry_state=ancestry_state, + closing_discipline_status=closing_discipline_status, ) ) if not head_sha: @@ -593,6 +636,8 @@ def evaluate_merge_gate( # noqa: C901, PLR0913 - explicit fail-closed admission passed=passed, body_narrative_status=body_narrative_status, body_not_ready_sentinels=body_not_ready_sentinels, + closing_discipline_status=closing_discipline_status, + closing_discipline_blockers=closing_discipline_blockers, reasons=reasons, ) @@ -618,6 +663,28 @@ def _gh(args: list[str], timeout: int = 30) -> subprocess.CompletedProcess: ) +def _fetch_live_closing_discipline( + pr_number: str | int, *, repo: str, body: str +) -> tuple[str, list[str]]: + """Recheck semantic closing references against current PR and issue metadata. + + The PR-contract status check runs on pull-request events, so a later incident + label or marker change could otherwise make its earlier green result stale. + Native merge admission repeats the check against the current commit list and + current issue metadata. An unavailable commit list is an explicit + fail-closed result. + """ + commit_messages = get_pr_commit_messages(str(pr_number), repo) + blockers = check_closes_discipline( + body, + repo, + commit_messages=commit_messages, + commit_messages_checked=True, + ) + status = "unavailable" if commit_messages is None else ("blocked" if blockers else "passed") + return status, blockers + + def _parse_json(stdout: str) -> tuple[Any, str | None]: """Parse JSON stdout into a Python object or return an error string.""" try: @@ -2072,6 +2139,7 @@ def _format_summary(audit: MergeGateAudit) -> str: f"- gate-verdict status: `{audit.gate_verdict_status}`", f"- PR metadata digest: `{audit.metadata_digest or '?'}`", f"- PR metadata verdict status: `{audit.metadata_verdict_status}`", + f"- closing-discipline status: `{audit.closing_discipline_status}`", f"- body narrative status: `{audit.body_narrative_status}`", f"- staleness verdict: `{audit.staleness_verdict}`", f"- CI conclusion: `{audit.ci_overall}`", @@ -2133,6 +2201,14 @@ def _evaluate_live( ) return _failed_audit(audit, "pr_snapshot_unavailable"), err + closing_discipline_status, closing_discipline_blockers = _fetch_live_closing_discipline( + pr_number, repo=repo, body=str(snapshot.get("body") or "") + ) + snapshot["closing_discipline"] = { + "status": closing_discipline_status, + "blockers": closing_discipline_blockers, + } + if merge_group_base_sha: # Inside the merge queue the base SHA is the prospective current main, so # staleness is fresh by construction; record the queue base as both base diff --git a/scripts/dev/single_account_merge_receipt.py b/scripts/dev/single_account_merge_receipt.py index c735bfd0cf..055a06211b 100644 --- a/scripts/dev/single_account_merge_receipt.py +++ b/scripts/dev/single_account_merge_receipt.py @@ -1738,6 +1738,7 @@ def build_live_evidence( ) -> tuple[dict[str, Any] | None, str | None]: """Re-read canonical merge-gate evidence for a report/validate/apply run.""" from scripts.dev.merge_queue_gate import ( # local import avoids a module cycle for pure helpers + _fetch_live_closing_discipline, evaluate_merge_gate, fetch_main_sha, fetch_pr_snapshot, @@ -1747,6 +1748,15 @@ def build_live_evidence( snapshot, error = fetch_pr_snapshot(pr_number, repo=repository) if error or not snapshot: return None, error or "PR snapshot unavailable" + closing_discipline_status, closing_discipline_blockers = _fetch_live_closing_discipline( + pr_number, + repo=repository, + body=str(snapshot.get("body") or ""), + ) + snapshot["closing_discipline"] = { + "status": closing_discipline_status, + "blockers": closing_discipline_blockers, + } current_base_sha = fetch_main_sha(repo=repository) if not current_base_sha: return None, "current main SHA unavailable" diff --git a/tests/dev/test_merge_queue_gate.py b/tests/dev/test_merge_queue_gate.py index 069783b02f..eb00816695 100644 --- a/tests/dev/test_merge_queue_gate.py +++ b/tests/dev/test_merge_queue_gate.py @@ -1408,6 +1408,11 @@ def test_evaluate_live_query_failure_preserves_unknown_thread_audit() -> None: "fetch_pr_snapshot", return_value=(snapshot, None), ), + patch.object( + merge_queue_gate_module, + "get_pr_commit_messages", + return_value="repair commit\n", + ), patch.object(merge_queue_gate_module, "fetch_main_sha", return_value=base_sha), patch.object( merge_queue_gate_module, @@ -1497,6 +1502,87 @@ def test_headgreen_merge_queue_strategy_fails_closed() -> None: assert "unsafe_merge_queue_strategy:HEADGREEN" in audit.reasons +def test_evaluate_merge_gate_requires_verified_closing_discipline() -> None: + """A live snapshot without a passing semantic-close recheck cannot be admitted.""" + body = "final body" + digest = metadata_digest("merge queue test PR", body) + audit = evaluate_merge_gate( + { + "number": 42, + "head_sha": FULL_SHA, + "labels": ["merge-ready"], + "draft": False, + "body": body, + "metadata_digest": digest, + "metadata_verdicts": [metadata_trailer(digest)], + "gate_verdicts": [f"gate-verdict: accepted @ {FULL_SHA}"], + "checks": {"overall": "success"}, + "changed_coverage": {"status": "success", "head_sha": FULL_SHA}, + "closing_discipline": { + "status": "unavailable", + "blockers": ["commit metadata unavailable"], + }, + }, + threads_resolved=True, + reviewers_requested=False, + ) + + assert audit.passed is False + assert audit.closing_discipline_status == "unavailable" + assert audit.closing_discipline_blockers == ["commit metadata unavailable"] + assert "closing_discipline_unavailable" in audit.reasons + + +def test_evaluate_live_carries_current_closing_discipline_result() -> None: + """The live evaluator binds the merge decision to the fresh contract recheck.""" + body = "final body" + digest = metadata_digest("live gate", body) + snapshot = { + "number": 42, + "title": "live gate", + "body": body, + "head_sha": FULL_SHA, + "base_sha": FULL_SHA, + "labels": ["merge-ready"], + "draft": False, + "checks": {"overall": "success"}, + "changed_coverage": {"status": "success", "head_sha": FULL_SHA}, + "gate_verdicts": [f"gate-verdict: accepted @ {FULL_SHA}"], + "metadata_digest": digest, + "metadata_verdicts": [metadata_trailer(digest)], + "reviewers_requested": False, + } + with ( + patch.object(merge_queue_gate_module, "fetch_pr_snapshot", return_value=(snapshot, None)), + patch.object(merge_queue_gate_module, "fetch_main_sha", return_value=FULL_SHA), + patch.object(merge_queue_gate_module, "fetch_threads_resolved", return_value=(True, None)), + patch.object( + merge_queue_gate_module, + "get_pr_commit_messages", + return_value="Closes: #8414", + ) as mock_commits, + patch.object( + merge_queue_gate_module, + "check_closes_discipline", + return_value=["incident blocker"], + ) as mock_check, + ): + audit, error = merge_queue_gate_module._evaluate_live(42, repo="owner/repo") + + assert error is None + assert audit.passed is False + assert audit.closing_discipline_status == "blocked" + assert audit.closing_discipline_blockers == ["incident blocker"] + assert "closing_discipline_blocked" in audit.reasons + mock_commits.assert_called_once_with("42", "owner/repo") + mock_check.assert_called_once_with( + body, + "owner/repo", + commit_messages="Closes: #8414", + commit_messages_checked=True, + ) + + def test_outstanding_requested_reviewer_fails_closed() -> None: """An explicit reviewer request receives the same fail-closed merger-preflight treatment.""" gate_verdict = f"gate-verdict: accepted @ {FULL_SHA}" @@ -1598,7 +1684,14 @@ def test_from_event_resolves_canonical_queue_ref_and_binds_pr_head(tmp_path) -> gate_verdict = f"gate-verdict: accepted @ {FULL_SHA}" threads = _review_threads_payload(nodes=[], total_count=0, has_next_page=False) - with patch("scripts.dev.merge_queue_gate._gh") as mock_gh: + with ( + patch("scripts.dev.merge_queue_gate._gh") as mock_gh, + patch.object( + merge_queue_gate_module, + "get_pr_commit_messages", + return_value="repair commit\n", + ), + ): mock_gh.side_effect = [ _gh_response(stdout=json.dumps(_raw_pr(body=gate_verdict))), _gh_response(stdout=json.dumps({"base": {"sha": "stale_base_sha"}})), @@ -1631,7 +1724,14 @@ def test_from_event_accepts_branch_name_queue_ref(tmp_path) -> None: gate_verdict = f"gate-verdict: accepted @ {FULL_SHA}" threads = _review_threads_payload(nodes=[], total_count=0, has_next_page=False) - with patch("scripts.dev.merge_queue_gate._gh") as mock_gh: + with ( + patch("scripts.dev.merge_queue_gate._gh") as mock_gh, + patch.object( + merge_queue_gate_module, + "get_pr_commit_messages", + return_value="repair commit\n", + ), + ): mock_gh.side_effect = [ _gh_response(stdout=json.dumps(_raw_pr(body=gate_verdict))), _gh_response(stdout=json.dumps({"base": {"sha": "stale_base_sha"}})), diff --git a/tests/dev/test_single_account_merge_receipt.py b/tests/dev/test_single_account_merge_receipt.py index 7e62170de1..fbf987dd5a 100644 --- a/tests/dev/test_single_account_merge_receipt.py +++ b/tests/dev/test_single_account_merge_receipt.py @@ -559,6 +559,11 @@ def to_dict() -> dict[str, Any]: monkeypatch.setattr( merge_queue_gate, "fetch_threads_resolved", lambda *args, **kwargs: (True, None) ) + monkeypatch.setattr( + merge_queue_gate, + "_fetch_live_closing_discipline", + lambda *args, **kwargs: ("passed", []), + ) monkeypatch.setattr( merge_queue_gate, "evaluate_merge_gate", lambda *args, **kwargs: StaleOnlyGate() ) @@ -791,6 +796,10 @@ def test_build_live_evidence_reports_rest_facts_when_thread_graphql_is_unavailab "scripts.dev.merge_queue_gate.fetch_threads_resolved", lambda *args, **kwargs: (None, "GitHub GraphQL quota exhausted"), ) + monkeypatch.setattr( + "scripts.dev.merge_queue_gate._fetch_live_closing_discipline", + lambda *args, **kwargs: ("blocked", ["incident blocker"]), + ) evidence, error = build_live_evidence(42, repository="owner/repo") @@ -803,6 +812,8 @@ def test_build_live_evidence_reports_rest_facts_when_thread_graphql_is_unavailab "diagnostic": "GitHub GraphQL quota exhausted", } assert evidence["thread_resolution"]["status"] == "unavailable" + assert evidence["gate_audit"]["closing_discipline_status"] == "blocked" + assert "closing_discipline_blocked" in evidence["gate_audit"]["reasons"] assert evidence["gate_audit"]["thread_resolution"] == "not_evaluated" assert "review_threads_not_evaluated" in evidence["gate_audit"]["reasons"] diff --git a/tests/validation/test_pr_contract_check.py b/tests/validation/test_pr_contract_check.py index da3d54bddd..212f1457d3 100644 --- a/tests/validation/test_pr_contract_check.py +++ b/tests/validation/test_pr_contract_check.py @@ -19,6 +19,10 @@ ROOT = Path(__file__).resolve().parents[2] +# PR #8440 is the known pre-guard regression: its merge reference closed the +# incident in #8414 before the two-green reconciler criterion was established. +KNOWN_HISTORICAL_MAIN_CI_CLOSING_GUARD_HITS = {8440: {"8414"}} + def _valid_review_sidecar(artifact: Path, artifact_path: str) -> dict[str, object]: """Build a valid immutable-evidence review sidecar payload for a fixture artifact.""" @@ -34,10 +38,17 @@ def _valid_review_sidecar(artifact: Path, artifact_path: str) -> dict[str, objec def test_find_closed_issues() -> None: """Test find_closed_issues matches closing keywords.""" body = ( - "This fixes #123 and closes #456. Resolves https://github.com/ll7/robot_sf_ll7/issues/789." + "This fixes #123, closes: #456, and resolves ll7/robot_sf_ll7#789. " + "Fixes https://github.com/ll7/robot_sf_ll7/issues/1011." ) closed = pr_contract_check.find_closed_issues(body) - assert closed == ["123", "456", "789"] + assert closed == ["123", "456", "789", "1011"] + + +def test_find_closed_issues_keeps_cross_repository_references_parseable() -> None: + """Qualified references are parsed so the discipline rule can ignore other repos.""" + body = "Closes other-org/other-repo#123 and closes ll7/robot_sf_ll7#456" + assert pr_contract_check.find_closed_issues(body) == ["123", "456"] def test_find_title_issues() -> None: @@ -48,25 +59,168 @@ def test_find_title_issues() -> None: def test_has_declaration_for_issue() -> None: """Test has_declaration_for_issue checks body matches.""" - body = "We reference Refs #123 here." + body = ( + "We reference Refs #123 here, close: #456, and fix ll7/robot_sf_ll7#789. " + "Resolves https://github.com/ll7/robot_sf_ll7/issues/1011." + ) assert pr_contract_check.has_declaration_for_issue("123", body) is True - assert pr_contract_check.has_declaration_for_issue("456", body) is False + assert pr_contract_check.has_declaration_for_issue("456", body) is True + assert pr_contract_check.has_declaration_for_issue("789", body) is True + assert pr_contract_check.has_declaration_for_issue("1011", body) is True + assert pr_contract_check.has_declaration_for_issue("999", body) is False @patch("subprocess.run") def test_check_closes_discipline(mock_run: MagicMock) -> None: - """Test check_closes_discipline blocks closing epic issues.""" + """Test check_closes_discipline protects special issue lifecycles.""" # Test case 1: Issue has no epic label - mock_run.return_value = MagicMock(returncode=0, stdout='{"labels": [{"name": "bug"}]}') + mock_run.return_value = MagicMock( + returncode=0, stdout='{"labels": [{"name": "bug"}], "body": ""}' + ) blockers = pr_contract_check.check_closes_discipline("Closes #123", "ll7/robot_sf_ll7") assert not blockers # Test case 2: Issue has epic label - mock_run.return_value = MagicMock(returncode=0, stdout='{"labels": [{"name": "epic"}]}') + mock_run.return_value = MagicMock( + returncode=0, stdout='{"labels": [{"name": "epic"}], "body": ""}' + ) blockers = pr_contract_check.check_closes_discipline("Closes #123", "ll7/robot_sf_ll7") assert len(blockers) == 1 assert "epic" in blockers[0] + # A canonical marker blocks all semantic closing keywords, including a repair PR. + mock_run.return_value = MagicMock( + returncode=0, + stdout=json.dumps( + {"labels": [], "body": "\nAutomated incident."} + ), + ) + blockers = pr_contract_check.check_closes_discipline("Fixes #8414", "ll7/robot_sf_ll7") + assert len(blockers) == 1 + assert "main continuous-integration (CI) incident" in blockers[0] + assert "Refs #8414" in blockers[0] + assert "two consecutive decisive green runs" in blockers[0] + + # The compatibility label protects marker-less incidents as well. + mock_run.return_value = MagicMock( + returncode=0, + stdout=json.dumps({"labels": [{"name": "ll7-main-red-incident:v1"}], "body": ""}), + ) + blockers = pr_contract_check.check_closes_discipline("Resolves #8441", "ll7/robot_sf_ll7") + assert len(blockers) == 1 + assert "main continuous-integration (CI) incident" in blockers[0] + + # A failed metadata read is unknown, not evidence that semantic closure is safe. + mock_run.return_value = MagicMock(returncode=1, stdout="", stderr="API unavailable") + blockers = pr_contract_check.check_closes_discipline("Closes #999", "ll7/robot_sf_ll7") + assert len(blockers) == 1 + assert "fails closed" in blockers[0] + + +@patch("scripts.ci.pr_contract_check.get_issue_metadata") +def test_check_closes_discipline_scans_commit_messages(mock_metadata: MagicMock) -> None: + """Commit-message closing keywords receive the same lifecycle protection as body keywords.""" + mock_metadata.return_value = ( + [], + "\nAutomated incident.", + ) + + blockers = pr_contract_check.check_closes_discipline( + "Adds a repair without a body closing keyword.", + "ll7/robot_sf_ll7", + commit_messages="Implement repair\n\nCloses: #8414\n", + commit_messages_checked=True, + ) + + assert len(blockers) == 1 + assert "PR commit message" in blockers[0] + mock_metadata.assert_called_once_with("8414", "ll7/robot_sf_ll7") + + +@patch("scripts.ci.pr_contract_check.get_issue_metadata") +def test_check_closes_discipline_ignores_other_repository(mock_metadata: MagicMock) -> None: + """A qualified close for another repository is not a local lifecycle mutation.""" + blockers = pr_contract_check.check_closes_discipline( + "Closes other-org/other-repo#8414", + "ll7/robot_sf_ll7", + ) + + assert not blockers + mock_metadata.assert_not_called() + + +@patch("scripts.ci.pr_contract_check.subprocess.run") +def test_get_issue_metadata_requires_complete_payload(mock_run: MagicMock) -> None: + """Partial issue responses cannot be treated as evidence that closure is safe.""" + mock_run.return_value = MagicMock(returncode=0, stdout='{"labels": []}') + assert pr_contract_check.get_issue_metadata("8414", "ll7/robot_sf_ll7") is None + + mock_run.return_value = MagicMock(returncode=0, stdout='{"body": ""}') + assert pr_contract_check.get_issue_metadata("8414", "ll7/robot_sf_ll7") is None + + +@patch("scripts.ci.pr_contract_check.subprocess.run") +def test_get_pr_commit_messages_uses_paginated_commit_api(mock_run: MagicMock) -> None: + """The commit source is fetched through the paginated PR commits endpoint.""" + mock_run.return_value = MagicMock(returncode=0, stdout="first\nsecond\n") + + assert pr_contract_check.get_pr_commit_messages("8451", "ll7/robot_sf_ll7") == "first\nsecond\n" + mock_run.assert_called_once_with( + [ + "gh", + "api", + "--paginate", + "repos/ll7/robot_sf_ll7/pulls/8451/commits?per_page=100", + "--jq", + ".[] | .commit.message", + ], + capture_output=True, + text=True, + timeout=30, + check=False, + ) + + +@patch("scripts.ci.pr_contract_check.subprocess.run") +def test_get_pr_commit_messages_rejects_empty_success(mock_run: MagicMock) -> None: + """A successful empty response is unavailable evidence, not a verified commit list.""" + mock_run.return_value = MagicMock(returncode=0, stdout=" \n") + + assert pr_contract_check.get_pr_commit_messages("8451", "ll7/robot_sf_ll7") is None + + +@patch("scripts.ci.pr_contract_check.get_issue_metadata") +def test_check_closes_discipline_fails_closed_when_commit_source_unavailable( + mock_metadata: MagicMock, +) -> None: + """A live PR check cannot silently skip commit-message closure references.""" + for commit_messages in (None, "", " \n"): + blockers = pr_contract_check.check_closes_discipline( + "No semantic closing reference in the body.", + "ll7/robot_sf_ll7", + commit_messages=commit_messages, + commit_messages_checked=True, + ) + + assert len(blockers) == 1 + assert "commit messages" in blockers[0] + mock_metadata.assert_not_called() + + +def test_check_closes_discipline_allows_non_closing_reference() -> None: + """``Refs`` keeps GitHub from closing an incident before reconciliation.""" + assert not pr_contract_check.check_closes_discipline("Refs #8414", "ll7/robot_sf_ll7") + + +def test_build_comment_body_marks_main_ci_closing_guard_failure() -> None: + """The summary row reports incident-closure blockers as failed.""" + blocker = ( + f"BLOCKER: {pr_contract_check.CLOSES_DISCIPLINE_TAG} PR body attempts to close " + "a canonical main-CI incident." + ) + comment = pr_contract_check.build_comment_body([blocker], [], [], "🔴 FAILED") + assert "| 1. Closes-discipline | ❌ FAILED |" in comment + def test_check_closure_declaration() -> None: """Test check_closure_declaration warns on missing declarations.""" @@ -567,7 +721,26 @@ def test_regression_last_20_merged_prs() -> None: blockers, _, _ = pr_contract_check.run_all_checks( title, body, changed_files, "ll7/robot_sf_ll7", "origin/main", None ) - assert not blockers, f"PR #{number} ('{title}') triggered false blockers: {blockers}" + metadata_unavailable = next( + (blocker for blocker in blockers if "Could not verify issue" in blocker), None + ) + if metadata_unavailable is not None: + # The production rule intentionally fails closed. A local regression sweep must not + # turn a temporary GitHub/API rate limit into a false code failure. + pytest.skip(f"Skipping live PR regression sweep: {metadata_unavailable}") + expected_incident_issues = KNOWN_HISTORICAL_MAIN_CI_CLOSING_GUARD_HITS.get(number, set()) + for issue in expected_incident_issues: + assert any(f"incident issue #{issue}" in blocker for blocker in blockers), ( + f"PR #{number} no longer exposes its known historical guard hit" + ) + unexpected_blockers = [ + blocker + for blocker in blockers + if not any(f"incident issue #{issue}" in blocker for issue in expected_incident_issues) + ] + assert not unexpected_blockers, ( + f"PR #{number} ('{title}') triggered unexpected blockers: {unexpected_blockers}" + ) class TestPlaceholderDocstringRatchet: