diff --git a/scripts/dev-prepush.sh b/scripts/dev-prepush.sh index 5144e50dd0..039528629d 100755 --- a/scripts/dev-prepush.sh +++ b/scripts/dev-prepush.sh @@ -61,7 +61,7 @@ while IFS= read -r commit; do if ! git diff-tree --no-commit-id --name-only -r "$commit" | \ PULSE_ALLOW_CONTRACT_NEUTRAL_COMMIT="$reason" \ python3 scripts/release_control/canonical_completion_guard.py \ - --files-from-stdin --diff-base "${commit}^"; then + --files-from-stdin --diff-base "${commit}^" --commit "$commit"; then fail "canonical completion guard @ $commit" fi done < <(git rev-list --reverse --no-merges "$BASE"..HEAD) diff --git a/scripts/release_control/canonical_completion_guard.py b/scripts/release_control/canonical_completion_guard.py index a96825452f..4fb6f81a0a 100644 --- a/scripts/release_control/canonical_completion_guard.py +++ b/scripts/release_control/canonical_completion_guard.py @@ -519,17 +519,18 @@ def contract_texts_have_substantive_change(base_text: str, staged_text: str) -> return False -def resolve_diff_base(ref: str) -> str: - """Resolve a diff-base ref to its merge base with HEAD. +def resolve_diff_base(ref: str, revision: str = "HEAD") -> str: + """Resolve a diff-base ref to its merge base with the evaluated revision. - 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. + A caller may pass a range base whose changed-file list was computed with + a three-dot diff (`base...revision`), 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). + with the file list; for the per-commit form (`^` against + ``) it is simply the parent. 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"], + ["git", "merge-base", ref, revision], cwd=REPO_ROOT, check=False, capture_output=True, @@ -539,14 +540,24 @@ def resolve_diff_base(ref: str) -> str: 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: +def staged_contract_has_substantive_change( + path: str, + diff_base: str | None = None, + revision: str = "HEAD", +) -> 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. + Both modes compare the evaluated change against its own parent. 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 against `revision`, the commit whose file list is being checked: + in a CI checkout the index equals HEAD, so the index comparison is always + empty there and would misreport every contract update as insubstantial. + HEAD is the right new side only when the evaluated commit is checked out. + CI and the pre-push check walk a branch's commits with HEAD at its tip, + where comparing against HEAD judged each commit by the whole branch: a + later commit reverting a contract paragraph blocked the commit that wrote + it, and a later contract update let an earlier bare runtime change pass. """ if diff_base is None: return contract_texts_have_substantive_change( @@ -555,7 +566,7 @@ def staged_contract_has_substantive_change(path: str, diff_base: str | None = No ) return contract_texts_have_substantive_change( git_blob_text(f"{diff_base}:{path}"), - git_blob_text(f"HEAD:{path}"), + git_blob_text(f"{revision}:{path}"), ) @@ -655,6 +666,7 @@ def check_staged_contracts( staged_files: Sequence[str], *, diff_base: str | None = None, + revision: str = "HEAD", inferred_override_reason: str | None = None, ) -> int: staged_set: Set[str] = set(staged_files) @@ -673,7 +685,7 @@ def check_staged_contracts( 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, diff_base) + if not staged_contract_has_substantive_change(contract_path, diff_base, revision) } missing_verification: Dict[str, dict] = {} for subsystem_id, data in impacted.items(): @@ -749,16 +761,19 @@ def parse_args(argv: Sequence[str]) -> argparse.Namespace: 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." + "from a commit range; requires --files-from-stdin. Pass --commit " + "too unless HEAD is the commit being evaluated." ), ) parser.add_argument( "--commit", default=None, help=( - "Committed revision represented by the changed-file list. In CI this " - "permits a fail-closed byte comparison for mechanically verified " - "Prettier-only frontend reformats." + "Committed revision represented by the changed-file list. With " + "--diff-base, contract texts are compared against this revision " + "instead of HEAD. In CI this also permits a fail-closed byte " + "comparison for mechanically verified Prettier-only frontend " + "reformats." ), ) args = parser.parse_args(list(argv)) @@ -771,13 +786,15 @@ def parse_args(argv: Sequence[str]) -> argparse.Namespace: 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 + revision = args.commit or "HEAD" + diff_base = resolve_diff_base(args.diff_base, revision) if args.diff_base else None if args.files_from_stdin: files = stdin_files(sys.stdin) inferred_reason = prettier_only_contract_neutral_reason(files, commit=args.commit) return check_staged_contracts( files, diff_base=diff_base, + revision=revision, inferred_override_reason=inferred_reason, ) 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 96d751be47..ca56cb36ad 100644 --- a/scripts/release_control/canonical_completion_guard_test.py +++ b/scripts/release_control/canonical_completion_guard_test.py @@ -18,6 +18,7 @@ from canonical_completion_guard import ( is_ignored_runtime_file, is_test_or_fixture, load_subsystem_rules, + main, path_policy_matches, parse_args, prettier_only_contract_neutral_reason, @@ -3730,9 +3731,9 @@ class ContractNeutralOverrideTest(unittest.TestCase): 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.""" + staged (index == HEAD), so it must compare the diff base against the + evaluated commit 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" @@ -3778,10 +3779,10 @@ class DiffBaseContractComparisonTest(unittest.TestCase): ) def test_check_staged_contracts_threads_diff_base_into_comparison(self): - seen: list[tuple[str, str | None]] = [] + seen: list[tuple[str, str | None, str]] = [] - def record(path, diff_base=None): - seen.append((path, diff_base)) + def record(path, diff_base=None, revision="HEAD"): + seen.append((path, diff_base, revision)) return True with ( @@ -3799,10 +3800,27 @@ class DiffBaseContractComparisonTest(unittest.TestCase): ), ): self.assertEqual( - check_staged_contracts([self.CONTRACT_PATH], diff_base="0d787a4b1"), + check_staged_contracts( + [self.CONTRACT_PATH], + diff_base="0d787a4b1", + revision="5e1f00d2c", + ), 0, ) - self.assertEqual(seen, [(self.CONTRACT_PATH, "0d787a4b1")]) + self.assertEqual(seen, [(self.CONTRACT_PATH, "0d787a4b1", "5e1f00d2c")]) + + def test_diff_base_mode_compares_base_against_evaluated_revision(self): + blobs = { + f"0d787a4b1:{self.CONTRACT_PATH}": self.BASE_TEXT, + f"5e1f00d2c:{self.CONTRACT_PATH}": self.HEAD_TEXT, + f"HEAD:{self.CONTRACT_PATH}": self.BASE_TEXT, + } + with patch("canonical_completion_guard.git_blob_text", side_effect=blobs.get): + self.assertTrue( + staged_contract_has_substantive_change( + self.CONTRACT_PATH, "0d787a4b1", "5e1f00d2c" + ) + ) def test_resolve_diff_base_returns_merge_base_sha(self): head = subprocess.run( @@ -3819,6 +3837,131 @@ class DiffBaseContractComparisonTest(unittest.TestCase): self.assertEqual(resolve_diff_base(zero_sha), zero_sha) +class EvaluatedCommitContractComparisonTest(unittest.TestCase): + """CI and dev-prepush walk a branch's commits with HEAD at the branch tip + and evaluate each commit against its own parent, as the pre-commit hook + did. The contract-text comparison must read the evaluated commit, not + HEAD. Pulse PR 2494 failed CI because its second commit reverted a + Current State paragraph its first commit wrote, so parent-vs-HEAD saw no + contract change for a commit the pre-commit hook had correctly passed. + The same comparison let a bare runtime commit pass when a later commit + carried the contract update.""" + + CONTRACT_PATH = "docs/release-control/v6/internal/subsystems/performance-and-scalability.md" + RUNTIME_PATH = "frontend-modern/src/features/platformPage/usePlatformWindowedItems.ts" + ORIGINAL = ( + "## Contract Metadata\n\n" + "- Status: active\n\n" + "## Current State\n\n" + "Spacer geometry and the inverse scroll-to-index mapping must share one\n" + "representative item height. The controller samples leading siblings.\n" + ) + REWRITTEN = ORIGINAL.replace( + "The controller samples leading siblings.", + "A table runway measures every mounted row.", + ) + METADATA_ONLY = ORIGINAL.replace("- Status: active", "- Status: active (reviewed)") + + def setUp(self) -> None: + temp_dir = tempfile.TemporaryDirectory() + self.addCleanup(temp_dir.cleanup) + self.repo = Path(temp_dir.name) + # Hook runs export GIT_DIR/GIT_INDEX_FILE for the real repository; + # the guard's git calls must reach the temporary repository instead. + env_patch = patch.dict(os.environ, strip_local_git_env(os.environ.copy()), clear=True) + env_patch.start() + self.addCleanup(env_patch.stop) + root_patch = patch("canonical_completion_guard.REPO_ROOT", self.repo) + root_patch.start() + self.addCleanup(root_patch.stop) + self.git("init", "-q", "-b", "main") + self.git("config", "user.name", "Pulse Test") + self.git("config", "user.email", "pulse-test@example.invalid") + self.commit_contract(self.ORIGINAL, "base") + + def git(self, *args: str) -> str: + return subprocess.run( + ["git", *args], + cwd=self.repo, + check=True, + capture_output=True, + text=True, + ).stdout.strip() + + def commit_contract(self, text: str, message: str) -> str: + contract = self.repo / self.CONTRACT_PATH + contract.parent.mkdir(parents=True, exist_ok=True) + contract.write_text(text, encoding="utf-8") + runtime = self.repo / self.RUNTIME_PATH + runtime.parent.mkdir(parents=True, exist_ok=True) + runtime.write_text(f"// {message}\n", encoding="utf-8") + self.git("add", self.CONTRACT_PATH, self.RUNTIME_PATH) + self.git("commit", "-q", "-m", message) + return self.git("rev-parse", "HEAD") + + def run_guard(self, commit: str) -> tuple[int, str]: + stderr = io.StringIO() + with ( + patch( + "canonical_completion_guard.infer_impacted_subsystems", + return_value={}, + ), + patch( + "canonical_completion_guard.required_contract_updates", + return_value={ + self.CONTRACT_PATH: { + "subsystem": "performance-and-scalability", + "touched_runtime_files": [self.RUNTIME_PATH], + } + }, + ), + patch.dict(os.environ, {CONTRACT_NEUTRAL_OVERRIDE_ENV: ""}), + patch("sys.stdin", io.StringIO(f"{self.CONTRACT_PATH}\n{self.RUNTIME_PATH}\n")), + redirect_stderr(stderr), + ): + status = main(["--files-from-stdin", "--diff-base", f"{commit}^", "--commit", commit]) + return status, stderr.getvalue() + + def test_later_revert_does_not_block_the_commit_that_wrote_the_contract(self) -> None: + wrote = self.commit_contract(self.REWRITTEN, "rewrite the runway paragraph") + self.commit_contract(self.ORIGINAL, "revert the runway paragraph") + + # HEAD now matches the parent of `wrote`; only the evaluated commit + # carries the substantive Current State change. + self.assertTrue( + staged_contract_has_substantive_change(self.CONTRACT_PATH, f"{wrote}^", wrote) + ) + self.assertFalse( + staged_contract_has_substantive_change(self.CONTRACT_PATH, f"{wrote}^", "HEAD") + ) + status, stderr = self.run_guard(wrote) + self.assertEqual(status, 0, stderr) + + def test_later_contract_update_does_not_pass_an_earlier_bare_commit(self) -> None: + bare = self.commit_contract(self.METADATA_ONLY, "touch only contract metadata") + self.commit_contract( + self.METADATA_ONLY.replace( + "The controller samples leading siblings.", + "A table runway measures every mounted row.", + ), + "record the runway paragraph", + ) + + status, stderr = self.run_guard(bare) + self.assertEqual(status, 1) + self.assertIn( + f"contract {self.CONTRACT_PATH} is staged but does not include a " + "substantive section update", + stderr, + ) + + def test_resolve_diff_base_anchors_on_the_evaluated_commit(self) -> None: + first = self.commit_contract(self.REWRITTEN, "first") + self.commit_contract(self.ORIGINAL, "second") + parent = self.git("rev-parse", f"{first}^") + self.assertEqual(resolve_diff_base(f"{first}^", first), parent) + + 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 @@ -3909,7 +4052,7 @@ import sys files = ",".join(line.strip() for line in sys.stdin if line.strip()) with open(os.environ["PULSE_TEST_GUARD_LOG"], "a", encoding="utf-8") as log: reason = os.environ.get("PULSE_ALLOW_CONTRACT_NEUTRAL_COMMIT", "").strip() - log.write(f"{reason}\\t{files}\\n") + log.write(f"{reason}\\t{files}\\t{' '.join(sys.argv[1:])}\\n") """, encoding="utf-8", ) @@ -3955,9 +4098,25 @@ with open(os.environ["PULSE_TEST_GUARD_LOG"], "a", encoding="utf-8") as log: ) self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + first, second = ( + subprocess.run( + ["git", "rev-parse", ref], + cwd=repo, + check=True, + capture_output=True, + text=True, + env=env, + ).stdout.strip() + for ref in ("HEAD^", "HEAD") + ) + # Each commit is judged as itself, not as the branch tip. self.assertEqual( guard_log.read_text(encoding="utf-8").splitlines(), - ["dependency-only test\tfirst.txt", "\tsecond.txt"], + [ + "dependency-only test\tfirst.txt\t" + f"--files-from-stdin --diff-base {first}^ --commit {first}", + f"\tsecond.txt\t--files-from-stdin --diff-base {second}^ --commit {second}", + ], )