From 54a6118d17c38f76df890d1b37dfcbfa7db5deb4 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Wed, 8 Jul 2026 14:35:07 +0100 Subject: [PATCH] Fix CI false positives in the canonical completion guard The guard judged substantive contract updates by diffing HEAD against the index. In CI nothing is staged, the index equals HEAD, so every contract file piped in via --files-from-stdin looked unchanged and the guard blocked compliant pushes. Concretely, run 28944317805 blocked 7645965af even though its deployment-installability.md addition sits inside the Current State section. The guard now accepts --diff-base (requires --files-from-stdin), resolves it to its merge base with HEAD so the comparison anchor matches the three-dot changed-file list, and compares base vs HEAD contract texts in that mode. Pre-commit keeps the index comparison. The canonical-governance workflow passes the push or PR range base. --- .github/workflows/canonical-governance.yml | 12 +- .../canonical_completion_guard.py | 58 +++++++++- .../canonical_completion_guard_test.py | 107 ++++++++++++++++++ 3 files changed, 168 insertions(+), 9 deletions(-) diff --git a/.github/workflows/canonical-governance.yml b/.github/workflows/canonical-governance.yml index c2df110e2..d04419ad3 100644 --- a/.github/workflows/canonical-governance.yml +++ b/.github/workflows/canonical-governance.yml @@ -83,9 +83,15 @@ jobs: shell: bash run: | set -euo pipefail - if [ -n "${{ steps.diff.outputs.range }}" ]; then - git diff --name-only "${{ steps.diff.outputs.range }}" \ - | python3 scripts/release_control/canonical_completion_guard.py --files-from-stdin + range="${{ steps.diff.outputs.range }}" + if [ -n "${range}" ]; then + # Pass the range base so the guard compares contract texts + # base-vs-HEAD; the CI index equals HEAD, so the default + # index comparison would misreport every contract update + # in the range as insubstantial. + git diff --name-only "${range}" \ + | python3 scripts/release_control/canonical_completion_guard.py \ + --files-from-stdin --diff-base "${range%%...*}" else printf '' | python3 scripts/release_control/canonical_completion_guard.py --files-from-stdin fi diff --git a/scripts/release_control/canonical_completion_guard.py b/scripts/release_control/canonical_completion_guard.py index aa18034a8..ad2e96bd9 100644 --- a/scripts/release_control/canonical_completion_guard.py +++ b/scripts/release_control/canonical_completion_guard.py @@ -475,10 +475,43 @@ def contract_texts_have_substantive_change(base_text: str, staged_text: str) -> return False -def staged_contract_has_substantive_change(path: str) -> bool: +def resolve_diff_base(ref: str) -> str: + """Resolve a diff-base ref to its merge base with HEAD. + + CI passes the push/PR range base, and the changed-file list is computed + with a three-dot diff (`base...head`), which diffs from the merge base. + Using the same anchor here keeps the contract-text comparison consistent + with the file list. Falls back to the raw ref when the merge base cannot + be resolved (e.g. a zero SHA on branch creation). + """ + result = subprocess.run( + ["git", "merge-base", ref, "HEAD"], + cwd=REPO_ROOT, + check=False, + capture_output=True, + text=True, + ) + merge_base = result.stdout.strip() + return merge_base if result.returncode == 0 and merge_base else ref + + +def staged_contract_has_substantive_change(path: str, diff_base: str | None = None) -> bool: + """Return True if the contract's change includes a substantive section edit. + + Without a diff base this compares HEAD against the index — the pre-commit + mode, where the pending change is staged. With a diff base (CI mode) it + compares the base commit against HEAD: in a CI checkout the index equals + HEAD, so the index comparison is always empty there and would misreport + every contract update as insubstantial. + """ + if diff_base is None: + return contract_texts_have_substantive_change( + git_blob_text(f"HEAD:{path}"), + git_blob_text(f":0:{path}"), + ) return contract_texts_have_substantive_change( + git_blob_text(f"{diff_base}:{path}"), git_blob_text(f"HEAD:{path}"), - git_blob_text(f":0:{path}"), ) @@ -574,7 +607,7 @@ def format_missing_requirements( return "\n".join(lines) -def check_staged_contracts(staged_files: Sequence[str]) -> int: +def check_staged_contracts(staged_files: Sequence[str], *, diff_base: str | None = None) -> int: staged_set: Set[str] = set(staged_files) impacted = infer_impacted_subsystems(staged_files, use_staged_registry=True) required_contracts = required_contract_updates( @@ -591,7 +624,7 @@ def check_staged_contracts(staged_files: Sequence[str]) -> int: contract_path: data for contract_path, data in required_contracts.items() if contract_path in staged_set - if not staged_contract_has_substantive_change(contract_path) + if not staged_contract_has_substantive_change(contract_path, diff_base) } missing_verification: Dict[str, dict] = {} for subsystem_id, data in impacted.items(): @@ -653,13 +686,26 @@ def parse_args(argv: Sequence[str]) -> argparse.Namespace: action="store_true", help="Read newline-delimited changed files from standard input instead of git staged files.", ) - return parser.parse_args(list(argv)) + parser.add_argument( + "--diff-base", + default=None, + help=( + "Commit-ish to compare contract texts against instead of the index. " + "Use in CI, where nothing is staged and the changed-file list comes " + "from a commit range; requires --files-from-stdin." + ), + ) + args = parser.parse_args(list(argv)) + if args.diff_base and not args.files_from_stdin: + parser.error("--diff-base requires --files-from-stdin") + return args def main(argv: Sequence[str] | None = None) -> int: args = parse_args(list(argv or ())) + diff_base = resolve_diff_base(args.diff_base) if args.diff_base else None if args.files_from_stdin: - return check_staged_contracts(stdin_files(sys.stdin)) + return check_staged_contracts(stdin_files(sys.stdin), diff_base=diff_base) return check_staged_contracts(git_staged_files()) diff --git a/scripts/release_control/canonical_completion_guard_test.py b/scripts/release_control/canonical_completion_guard_test.py index 3ac56396e..e15539db3 100644 --- a/scripts/release_control/canonical_completion_guard_test.py +++ b/scripts/release_control/canonical_completion_guard_test.py @@ -1,5 +1,6 @@ import io import os +import subprocess import unittest from contextlib import redirect_stderr from pathlib import Path @@ -19,6 +20,8 @@ from canonical_completion_guard import ( path_policy_matches, parse_args, required_contract_updates, + resolve_diff_base, + staged_contract_has_substantive_change, staged_verification_files_for_requirement, stdin_files, subsystem_matches_path, @@ -3069,6 +3072,18 @@ None yet. def test_parse_args_supports_files_from_stdin(self): args = parse_args(["--files-from-stdin"]) self.assertTrue(args.files_from_stdin) + self.assertIsNone(args.diff_base) + + def test_parse_args_supports_diff_base_with_files_from_stdin(self): + args = parse_args(["--files-from-stdin", "--diff-base", "0d787a4b1"]) + self.assertTrue(args.files_from_stdin) + self.assertEqual(args.diff_base, "0d787a4b1") + + def test_parse_args_rejects_diff_base_without_files_from_stdin(self): + stderr = io.StringIO() + with redirect_stderr(stderr), self.assertRaises(SystemExit): + parse_args(["--diff-base", "0d787a4b1"]) + self.assertIn("--diff-base requires --files-from-stdin", stderr.getvalue()) def test_explicit_coverage_subsystems_have_no_unmatched_runtime_files(self): explicit_rules = { @@ -3201,6 +3216,98 @@ class ContractNeutralOverrideTest(unittest.TestCase): self.assertIn("contract-neutral", stderr_value) +class DiffBaseContractComparisonTest(unittest.TestCase): + """The guard runs in two modes. Pre-commit compares HEAD against the + index, where the pending contract update is staged. CI has nothing + staged (index == HEAD), so it must compare the push-range base against + HEAD instead; before --diff-base existed, CI misreported every staged + contract as having no substantive change.""" + + CONTRACT_PATH = "docs/release-control/v6/internal/subsystems/deployment-installability.md" + BASE_TEXT = "## Current State\n\nExisting obligation.\n" + HEAD_TEXT = "## Current State\n\nExisting obligation.\nNew scoped exception recorded.\n" + + def test_index_mode_compares_head_against_index(self): + blobs = { + f"HEAD:{self.CONTRACT_PATH}": self.BASE_TEXT, + f":0:{self.CONTRACT_PATH}": self.HEAD_TEXT, + } + with patch("canonical_completion_guard.git_blob_text", side_effect=blobs.get): + self.assertTrue(staged_contract_has_substantive_change(self.CONTRACT_PATH)) + + def test_index_mode_reports_no_change_when_index_equals_head(self): + # The CI failure mode: index == HEAD, so the index comparison is + # empty even though the push range contains a substantive update. + blobs = { + f"HEAD:{self.CONTRACT_PATH}": self.HEAD_TEXT, + f":0:{self.CONTRACT_PATH}": self.HEAD_TEXT, + } + with patch("canonical_completion_guard.git_blob_text", side_effect=blobs.get): + self.assertFalse(staged_contract_has_substantive_change(self.CONTRACT_PATH)) + + def test_diff_base_mode_compares_base_against_head(self): + blobs = { + f"0d787a4b1:{self.CONTRACT_PATH}": self.BASE_TEXT, + f"HEAD:{self.CONTRACT_PATH}": self.HEAD_TEXT, + f":0:{self.CONTRACT_PATH}": self.HEAD_TEXT, + } + with patch("canonical_completion_guard.git_blob_text", side_effect=blobs.get): + self.assertTrue( + staged_contract_has_substantive_change(self.CONTRACT_PATH, "0d787a4b1") + ) + + def test_diff_base_mode_reports_no_change_for_identical_texts(self): + blobs = { + f"0d787a4b1:{self.CONTRACT_PATH}": self.HEAD_TEXT, + f"HEAD:{self.CONTRACT_PATH}": self.HEAD_TEXT, + } + with patch("canonical_completion_guard.git_blob_text", side_effect=blobs.get): + self.assertFalse( + staged_contract_has_substantive_change(self.CONTRACT_PATH, "0d787a4b1") + ) + + def test_check_staged_contracts_threads_diff_base_into_comparison(self): + seen: list[tuple[str, str | None]] = [] + + def record(path, diff_base=None): + seen.append((path, diff_base)) + return True + + with ( + patch( + "canonical_completion_guard.infer_impacted_subsystems", + return_value={}, + ), + patch( + "canonical_completion_guard.required_contract_updates", + return_value={self.CONTRACT_PATH: {"subsystem": "deployment-installability"}}, + ), + patch( + "canonical_completion_guard.staged_contract_has_substantive_change", + side_effect=record, + ), + ): + self.assertEqual( + check_staged_contracts([self.CONTRACT_PATH], diff_base="0d787a4b1"), + 0, + ) + self.assertEqual(seen, [(self.CONTRACT_PATH, "0d787a4b1")]) + + def test_resolve_diff_base_returns_merge_base_sha(self): + head = subprocess.run( + ["git", "rev-parse", "HEAD"], + cwd=REPO_ROOT, + check=True, + capture_output=True, + text=True, + ).stdout.strip() + self.assertEqual(resolve_diff_base("HEAD"), head) + + def test_resolve_diff_base_falls_back_to_raw_ref(self): + zero_sha = "0" * 40 + self.assertEqual(resolve_diff_base(zero_sha), zero_sha) + + class ReleaseCycleArtifactGuardTest(unittest.TestCase): """The release_cycle_artifact_globs field in a subsystem rule lets per-RC-cycle artifacts (packet drafts, version-pointer docs, regenerated