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