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 <ref> (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.
This commit is contained in:
rcourtman 2026-07-08 14:35:07 +01:00
parent 0a9a29d63d
commit 54a6118d17
3 changed files with 168 additions and 9 deletions

View file

@ -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

View file

@ -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())

View file

@ -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