mirror of
https://github.com/rcourtman/Pulse.git
synced 2026-07-09 16:00:59 +00:00
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:
parent
0a9a29d63d
commit
54a6118d17
3 changed files with 168 additions and 9 deletions
12
.github/workflows/canonical-governance.yml
vendored
12
.github/workflows/canonical-governance.yml
vendored
|
|
@ -83,9 +83,15 @@ jobs:
|
||||||
shell: bash
|
shell: bash
|
||||||
run: |
|
run: |
|
||||||
set -euo pipefail
|
set -euo pipefail
|
||||||
if [ -n "${{ steps.diff.outputs.range }}" ]; then
|
range="${{ steps.diff.outputs.range }}"
|
||||||
git diff --name-only "${{ steps.diff.outputs.range }}" \
|
if [ -n "${range}" ]; then
|
||||||
| python3 scripts/release_control/canonical_completion_guard.py --files-from-stdin
|
# 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
|
else
|
||||||
printf '' | python3 scripts/release_control/canonical_completion_guard.py --files-from-stdin
|
printf '' | python3 scripts/release_control/canonical_completion_guard.py --files-from-stdin
|
||||||
fi
|
fi
|
||||||
|
|
|
||||||
|
|
@ -475,10 +475,43 @@ def contract_texts_have_substantive_change(base_text: str, staged_text: str) ->
|
||||||
return False
|
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(
|
return contract_texts_have_substantive_change(
|
||||||
|
git_blob_text(f"{diff_base}:{path}"),
|
||||||
git_blob_text(f"HEAD:{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)
|
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)
|
staged_set: Set[str] = set(staged_files)
|
||||||
impacted = infer_impacted_subsystems(staged_files, use_staged_registry=True)
|
impacted = infer_impacted_subsystems(staged_files, use_staged_registry=True)
|
||||||
required_contracts = required_contract_updates(
|
required_contracts = required_contract_updates(
|
||||||
|
|
@ -591,7 +624,7 @@ def check_staged_contracts(staged_files: Sequence[str]) -> int:
|
||||||
contract_path: data
|
contract_path: data
|
||||||
for contract_path, data in required_contracts.items()
|
for contract_path, data in required_contracts.items()
|
||||||
if contract_path in staged_set
|
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] = {}
|
missing_verification: Dict[str, dict] = {}
|
||||||
for subsystem_id, data in impacted.items():
|
for subsystem_id, data in impacted.items():
|
||||||
|
|
@ -653,13 +686,26 @@ def parse_args(argv: Sequence[str]) -> argparse.Namespace:
|
||||||
action="store_true",
|
action="store_true",
|
||||||
help="Read newline-delimited changed files from standard input instead of git staged files.",
|
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:
|
def main(argv: Sequence[str] | None = None) -> int:
|
||||||
args = parse_args(list(argv or ()))
|
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:
|
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())
|
return check_staged_contracts(git_staged_files())
|
||||||
|
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -1,5 +1,6 @@
|
||||||
import io
|
import io
|
||||||
import os
|
import os
|
||||||
|
import subprocess
|
||||||
import unittest
|
import unittest
|
||||||
from contextlib import redirect_stderr
|
from contextlib import redirect_stderr
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
|
|
@ -19,6 +20,8 @@ from canonical_completion_guard import (
|
||||||
path_policy_matches,
|
path_policy_matches,
|
||||||
parse_args,
|
parse_args,
|
||||||
required_contract_updates,
|
required_contract_updates,
|
||||||
|
resolve_diff_base,
|
||||||
|
staged_contract_has_substantive_change,
|
||||||
staged_verification_files_for_requirement,
|
staged_verification_files_for_requirement,
|
||||||
stdin_files,
|
stdin_files,
|
||||||
subsystem_matches_path,
|
subsystem_matches_path,
|
||||||
|
|
@ -3069,6 +3072,18 @@ None yet.
|
||||||
def test_parse_args_supports_files_from_stdin(self):
|
def test_parse_args_supports_files_from_stdin(self):
|
||||||
args = parse_args(["--files-from-stdin"])
|
args = parse_args(["--files-from-stdin"])
|
||||||
self.assertTrue(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):
|
def test_explicit_coverage_subsystems_have_no_unmatched_runtime_files(self):
|
||||||
explicit_rules = {
|
explicit_rules = {
|
||||||
|
|
@ -3201,6 +3216,98 @@ class ContractNeutralOverrideTest(unittest.TestCase):
|
||||||
self.assertIn("contract-neutral", stderr_value)
|
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):
|
class ReleaseCycleArtifactGuardTest(unittest.TestCase):
|
||||||
"""The release_cycle_artifact_globs field in a subsystem rule lets
|
"""The release_cycle_artifact_globs field in a subsystem rule lets
|
||||||
per-RC-cycle artifacts (packet drafts, version-pointer docs, regenerated
|
per-RC-cycle artifacts (packet drafts, version-pointer docs, regenerated
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue