From 842c1ddc1a633153c59eb8b465e638d955a56b02 Mon Sep 17 00:00:00 2001 From: chaofengw Date: Tue, 1 Sep 2026 07:55:23 +0000 Subject: [PATCH] fix(ci): allow main to advance during premerge Signed-off-by: chaofengw --- .github/workflows/internal-ci-bridge.yml | 29 ++--- tests/tools/test_github_actions_ci.py | 9 +- tests/tools/test_public_failure.py | 5 +- tests/tools/test_public_failure_identity.py | 117 +++++++++++++++++++ tools/public_failure/identity.py | 120 ++++++++++++++++++++ 5 files changed, 263 insertions(+), 17 deletions(-) create mode 100644 tests/tools/test_public_failure_identity.py create mode 100644 tools/public_failure/identity.py diff --git a/.github/workflows/internal-ci-bridge.yml b/.github/workflows/internal-ci-bridge.yml index 984f6fd6c..4a73e1771 100644 --- a/.github/workflows/internal-ci-bridge.yml +++ b/.github/workflows/internal-ci-bridge.yml @@ -338,6 +338,7 @@ jobs: env: PUBLIC_FAILURE_JSON: ${{ runner.temp }}/private-public-failure/public-failure.json PUBLIC_FAILURE_LOG: ${{ runner.temp }}/public-failure.log + GH_TOKEN: ${{ github.token }} EXPECTED_DISPATCH_NONCE: ${{ steps.private_run.outputs.dispatch_nonce }} EXPECTED_HEAD_SHA: ${{ needs.authorize.outputs.head_sha }} EXPECTED_BASE_SHA: ${{ needs.authorize.outputs.base_sha }} @@ -349,6 +350,7 @@ jobs: from pathlib import Path from tools.public_failure.contract import validate_public_failure + from tools.public_failure.identity import GitHubCommitGraph, validate_failure_identity from tools.public_failure.render import render_failure_report from tools.public_failure.safety import assert_public_payload_safe @@ -356,14 +358,16 @@ jobs: Path(os.environ["PUBLIC_FAILURE_JSON"]).read_text(encoding="utf-8") ) validate_public_failure(report) - expected = { - "dispatch_nonce": os.environ["EXPECTED_DISPATCH_NONCE"], - "head_sha": os.environ["EXPECTED_HEAD_SHA"], - "base_sha": os.environ["EXPECTED_BASE_SHA"], - "pr_number": int(os.environ["EXPECTED_PR_NUMBER"]), - } - if any(report.get(key) != value for key, value in expected.items()): - raise SystemExit("The failure payload does not match this authorized snapshot.") + graph = GitHubCommitGraph(os.environ["GITHUB_REPOSITORY"]) + validate_failure_identity( + report, + expected_dispatch_nonce=os.environ["EXPECTED_DISPATCH_NONCE"], + expected_head_sha=os.environ["EXPECTED_HEAD_SHA"], + expected_base_sha=os.environ["EXPECTED_BASE_SHA"], + expected_pr_number=int(os.environ["EXPECTED_PR_NUMBER"]), + commit_parents=graph.commit_parents, + is_ancestor=graph.is_ancestor, + ) document = render_failure_report(report) assert_public_payload_safe(report, document) Path(os.environ["PUBLIC_FAILURE_LOG"]).write_bytes(document) @@ -408,7 +412,6 @@ jobs: GH_TOKEN: ${{ github.token }} PR_NUMBER: ${{ needs.authorize.outputs.pr_number }} HEAD_SHA: ${{ needs.authorize.outputs.head_sha }} - BASE_SHA: ${{ needs.authorize.outputs.base_sha }} DISPATCH_RESULT: ${{ needs.dispatch.result }} PRIVATE_CONCLUSION: ${{ needs.dispatch.outputs.private_conclusion }} PUBLIC_LOG_READY: ${{ needs.dispatch.outputs.public_log_ready }} @@ -419,24 +422,22 @@ jobs: publish_comment=true use_validated_log=false + # Main is allowed to advance while this exact PR head is in CI. The + # private resolver separately binds testing to an authorized merge. snapshot_current=false if pull="$(gh api --method GET "/repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER")"; then current_state="$(jq -r '.state // ""' <<<"$pull")" current_repo="$(jq -r '.base.repo.full_name // ""' <<<"$pull")" current_ref="$(jq -r '.base.ref // ""' <<<"$pull")" current_head="$(jq -r '.head.sha // ""' <<<"$pull")" - current_base="$(jq -r '.base.sha // ""' <<<"$pull")" if [ "$current_state" = "open" ] \ && [ "$current_repo" = "$GITHUB_REPOSITORY" ] \ && [ "$current_ref" = "main" ] \ - && [ "$current_head" = "$HEAD_SHA" ] \ - && [ "$current_base" = "$BASE_SHA" ]; then + && [ "$current_head" = "$HEAD_SHA" ]; then snapshot_current=true elif [ "$current_head" != "$HEAD_SHA" ]; then description="Automated internal CI result was superseded by a newer PR head" publish_comment=false - elif [ "$current_base" != "$BASE_SHA" ]; then - description="The PR base changed during internal CI; rerun is required" fi fi diff --git a/tests/tools/test_github_actions_ci.py b/tests/tools/test_github_actions_ci.py index cc4077b5e..68f8bb247 100644 --- a/tests/tools/test_github_actions_ci.py +++ b/tests/tools/test_github_actions_ci.py @@ -506,8 +506,11 @@ def test_internal_ci_bridge_only_dispatches_an_exact_trusted_head() -> None: assert "--name public-failure-payload" in dispatch assert "--log" not in dispatch assert "validate_public_failure(report)" in dispatch + assert "validate_failure_identity(" in dispatch + assert "commit_parents=graph.commit_parents" in dispatch + assert "is_ancestor=graph.is_ancestor" in dispatch assert "EXPECTED_DISPATCH_NONCE" in dispatch - assert '"dispatch_nonce": os.environ["EXPECTED_DISPATCH_NONCE"]' in dispatch + assert 'expected_dispatch_nonce=os.environ["EXPECTED_DISPATCH_NONCE"]' in dispatch assert "assert_public_payload_safe(report, document)" in dispatch assert "name: public-failure-log" in dispatch assert "Automated internal CI failed; open the public failure log" in publish @@ -515,7 +518,9 @@ def test_internal_ci_bridge_only_dispatches_an_exact_trusted_head() -> None: assert "always() && needs.authorize.result == 'success'" in workflow assert "cancelled|timed_out|skipped|neutral|action_required" in workflow assert 'payload_size" -gt 65536' in dispatch - assert '"base_sha": os.environ["EXPECTED_BASE_SHA"]' in dispatch + assert '"base_sha": os.environ["EXPECTED_BASE_SHA"]' not in dispatch + assert "current_base" not in publish + assert "The PR base changed during internal CI" not in publish assert publish.index("- name: Publish the terminal automated status") < publish.index( "- name: Print public-failure.log" ) diff --git a/tests/tools/test_public_failure.py b/tests/tools/test_public_failure.py index f8c8e5650..067d7cb48 100644 --- a/tests/tools/test_public_failure.py +++ b/tests/tools/test_public_failure.py @@ -570,8 +570,11 @@ def test_internal_ci_bridge_publishes_the_private_sanitized_artifact() -> None: assert ( 'expected_title="Source PR #$PR_NUMBER · $HEAD_SHA · dispatch $dispatch_nonce"' in workflow ) - assert '"dispatch_nonce": os.environ["EXPECTED_DISPATCH_NONCE"]' in workflow + assert 'expected_dispatch_nonce=os.environ["EXPECTED_DISPATCH_NONCE"]' in workflow assert "validate_public_failure(report)" in workflow + assert "validate_failure_identity(" in workflow + assert "commit_parents=graph.commit_parents" in workflow + assert "is_ancestor=graph.is_ancestor" in workflow assert "assert_public_payload_safe(report, document)" in workflow assert "name: public-failure-log" in workflow assert workflow.count("TRTMC Internal CI / Automated premerge gate") == 2 diff --git a/tests/tools/test_public_failure_identity.py b/tests/tools/test_public_failure_identity.py new file mode 100644 index 000000000..a609adf67 --- /dev/null +++ b/tests/tools/test_public_failure_identity.py @@ -0,0 +1,117 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +from __future__ import annotations + +import subprocess +from collections.abc import Sequence + +import pytest + +from tools.public_failure.identity import ( + GitHubCommitGraph, + PublicFailureIdentityError, + validate_failure_identity, +) + + +APPROVED_BASE_SHA = "a" * 40 +CURRENT_MERGE_PARENT_SHA = "b" * 40 +HEAD_SHA = "c" * 40 +TESTED_MERGE_SHA = "d" * 40 +DISPATCH_NONCE = "e" * 32 + + +def _report(*, tested_revision_kind: str = "merge") -> dict[str, object]: + if tested_revision_kind == "head": + return { + "dispatch_nonce": DISPATCH_NONCE, + "pr_number": 1059, + "head_sha": HEAD_SHA, + "base_sha": APPROVED_BASE_SHA, + "tested_revision": HEAD_SHA, + "tested_revision_kind": "head", + } + return { + "dispatch_nonce": DISPATCH_NONCE, + "pr_number": 1059, + "head_sha": HEAD_SHA, + "base_sha": CURRENT_MERGE_PARENT_SHA, + "tested_revision": TESTED_MERGE_SHA, + "tested_revision_kind": "merge", + } + + +def _validate( + report: dict[str, object], + *, + parents: Sequence[str] = (CURRENT_MERGE_PARENT_SHA, HEAD_SHA), + approved_base_is_ancestor: bool = True, +) -> None: + validate_failure_identity( + report, + expected_dispatch_nonce=DISPATCH_NONCE, + expected_pr_number=1059, + expected_head_sha=HEAD_SHA, + expected_base_sha=APPROVED_BASE_SHA, + commit_parents=lambda _revision: parents, + is_ancestor=lambda _ancestor, _descendant: approved_base_is_ancestor, + ) + + +def test_accepts_merge_payload_based_on_a_newer_main_descendant() -> None: + _validate(_report()) + + +def test_rejects_merge_payload_outside_the_authorized_base_lineage() -> None: + with pytest.raises(PublicFailureIdentityError, match="authorized base lineage"): + _validate(_report(), approved_base_is_ancestor=False) + + +def test_rejects_merge_payload_with_the_wrong_commit_parents() -> None: + with pytest.raises(PublicFailureIdentityError, match="expected commit parents"): + _validate(_report(), parents=(APPROVED_BASE_SHA, HEAD_SHA)) + + +def test_accepts_head_payload_when_snapshot_resolution_failed() -> None: + def unexpected_graph_lookup(*_args: str) -> object: + raise AssertionError("head payload must not query the commit graph") + + validate_failure_identity( + _report(tested_revision_kind="head"), + expected_dispatch_nonce=DISPATCH_NONCE, + expected_pr_number=1059, + expected_head_sha=HEAD_SHA, + expected_base_sha=APPROVED_BASE_SHA, + commit_parents=unexpected_graph_lookup, + is_ancestor=unexpected_graph_lookup, + ) + + +def test_github_graph_requests_only_bounded_identity_fields( + monkeypatch: pytest.MonkeyPatch, +) -> None: + calls: list[list[str]] = [] + + def fake_run( + arguments: list[str], **_kwargs: object + ) -> subprocess.CompletedProcess[str]: + calls.append(arguments) + if "/commits/" in arguments[4]: + output = ( + f'{{"sha":"{TESTED_MERGE_SHA}",' + f'"parents":["{CURRENT_MERGE_PARENT_SHA}","{HEAD_SHA}"]}}' + ) + else: + output = f'{{"status":"ahead","merge_base_sha":"{APPROVED_BASE_SHA}"}}' + return subprocess.CompletedProcess(arguments, 0, stdout=output, stderr="") + + monkeypatch.setattr("tools.public_failure.identity.subprocess.run", fake_run) + graph = GitHubCommitGraph("NVIDIA/TensorRT-Model-Connect") + + assert graph.commit_parents(TESTED_MERGE_SHA) == ( + CURRENT_MERGE_PARENT_SHA, + HEAD_SHA, + ) + assert graph.is_ancestor(APPROVED_BASE_SHA, CURRENT_MERGE_PARENT_SHA) + assert all("--jq" in call for call in calls) diff --git a/tools/public_failure/identity.py b/tools/public_failure/identity.py new file mode 100644 index 000000000..c04dcb7ca --- /dev/null +++ b/tools/public_failure/identity.py @@ -0,0 +1,120 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Bind a private failure payload to one authorized Source commit graph.""" + +from __future__ import annotations + +import json +import subprocess +from collections.abc import Callable, Mapping, Sequence + + +class PublicFailureIdentityError(ValueError): + """The failure payload does not match the authorized Source snapshot.""" + + +CommitParents = Callable[[str], Sequence[str]] +IsAncestor = Callable[[str, str], bool] + + +def validate_failure_identity( + report: Mapping[str, object], + *, + expected_dispatch_nonce: str, + expected_pr_number: int, + expected_head_sha: str, + expected_base_sha: str, + commit_parents: CommitParents, + is_ancestor: IsAncestor, +) -> None: + """Validate exact run identity plus the authorized base-to-merge lineage.""" + expected = { + "dispatch_nonce": expected_dispatch_nonce, + "head_sha": expected_head_sha, + "pr_number": expected_pr_number, + } + if any(report.get(key) != value for key, value in expected.items()): + raise PublicFailureIdentityError("the failure payload does not match this authorized run") + + kind = report.get("tested_revision_kind") + tested_revision = report.get("tested_revision") + resolved_base = report.get("base_sha") + if kind == "head": + if tested_revision != expected_head_sha or resolved_base != expected_base_sha: + raise PublicFailureIdentityError( + "the head failure payload does not match the authorized snapshot" + ) + return + if ( + kind != "merge" + or not isinstance(tested_revision, str) + or not isinstance(resolved_base, str) + ): + raise PublicFailureIdentityError("the failure payload has an invalid revision kind") + + if not is_ancestor(expected_base_sha, resolved_base): + raise PublicFailureIdentityError( + "the resolved merge parent is outside the authorized base lineage" + ) + if tuple(commit_parents(tested_revision)) != (resolved_base, expected_head_sha): + raise PublicFailureIdentityError( + "the tested merge does not have the expected commit parents" + ) + + +class GitHubCommitGraph: + """Read the minimal public commit graph required for identity validation.""" + + def __init__(self, repository: str) -> None: + self.repository = repository + + def _api(self, endpoint: str, jq_filter: str) -> Mapping[str, object]: + result = subprocess.run( + ["gh", "api", "--method", "GET", endpoint, "--jq", jq_filter], + check=False, + capture_output=True, + text=True, + ) + if result.returncode: + raise PublicFailureIdentityError( + "GitHub could not verify the failure payload commit graph" + ) + try: + payload = json.loads(result.stdout) + except json.JSONDecodeError as error: + raise PublicFailureIdentityError( + "GitHub returned an invalid commit graph response" + ) from error + if not isinstance(payload, Mapping): + raise PublicFailureIdentityError("GitHub returned an invalid commit graph response") + return payload + + def commit_parents(self, revision: str) -> tuple[str, ...]: + payload = self._api( + f"/repos/{self.repository}/commits/{revision}", + "{sha: .sha, parents: [.parents[].sha]}", + ) + if payload.get("sha") != revision: + raise PublicFailureIdentityError("GitHub returned the wrong tested revision") + parents = payload.get("parents") + if not isinstance(parents, list): + raise PublicFailureIdentityError("GitHub returned invalid commit parents") + resolved: list[str] = [] + for parent in parents: + if not isinstance(parent, str): + raise PublicFailureIdentityError("GitHub returned invalid commit parents") + resolved.append(parent) + return tuple(resolved) + + def is_ancestor(self, ancestor: str, descendant: str) -> bool: + if ancestor == descendant: + return True + comparison = self._api( + f"/repos/{self.repository}/compare/{ancestor}...{descendant}", + "{status: .status, merge_base_sha: .merge_base_commit.sha}", + ) + return ( + comparison.get("status") == "ahead" + and comparison.get("merge_base_sha") == ancestor + )