Skip to content

fix(ci): reject PR runs after retargeting or target force-push #3442

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ShryukGrandhi wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(ci): reject PR runs after retargeting or target force-push #3442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions .github/scripts/ci-policy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import os
from pathlib import Path
import subprocess
from urllib.parse import quote

MAINLINE = {"server", "commons", "struct", "pd", "store", "hstore", "cluster", "docker", "helm",
"dependency_license", "macos", "security", "distributed_smoke"}
Expand Down Expand Up @@ -289,16 +290,21 @@ def require_current_pr(plan, fetch):
if (live.get("state") != "open" or live["head"]["sha"] != plan["head"]
or live["head"]["repo"]["full_name"] != plan["source"]
or live["head"]["ref"] != plan["branch"]
or live["base"]["repo"]["full_name"] != plan["repository"]):
or live["base"]["repo"]["full_name"] != plan["repository"]
or live["base"]["ref"] != plan["target"]):
raise StaleInputError("PR inputs changed; refresh the branch and start a new PR run")
# Target advancement is fine; a force-pushed target no longer contains the tested base.
compare = fetch(f"repos/{plan['repository']}/compare/{plan['base']}...{quote(plan['target'])}")
if compare["status"] not in {"ahead", "identical"}:
raise StaleInputError("PR target history changed; start a new PR run")


def create_plan(project, event, repository, fetch=api, event_name=None):
event_name = event_name or os.environ.get("GITHUB_EVENT_NAME") or (
"pull_request" if "pull_request" in event else "push")
paths = []
plan = {"schema": 1, "project": project, "repository": repository, "pr": 0,
"source": repository, "branch": "", "base": "", "head": git("rev-parse", "HEAD"),
"source": repository, "branch": "", "target": "", "base": "", "head": git("rev-parse", "HEAD"),
"reason": "affected inputs", "testedMergeSHA": git("rev-parse", "HEAD"), "event": event_name}
try:
pr = event.get("pull_request")
Expand All @@ -308,10 +314,11 @@ def create_plan(project, event, repository, fetch=api, event_name=None):
plan["pr"] = pr["number"]
try:
plan.update(source=pr["head"]["repo"]["full_name"], base=pr["base"]["sha"],
head=pr["head"]["sha"], branch=pr["head"]["ref"])
head=pr["head"]["sha"], branch=pr["head"]["ref"], target=pr["base"]["ref"])
except (KeyError, TypeError) as error:
raise StaleInputError("PR event lacks required input identity") from error
if not all(isinstance(plan[key], str) and plan[key] for key in ("source", "base", "head", "branch")):
if not all(isinstance(plan[key], str) and plan[key]
for key in ("source", "base", "head", "branch", "target")):
raise StaleInputError("PR event has an empty input identity")
parents = git("show", "-s", "--format=%P", plan["testedMergeSHA"]).split()
if (plan["testedMergeSHA"] != os.environ.get("GITHUB_SHA")
Expand Down
53 changes: 39 additions & 14 deletions .github/scripts/test_ci_policy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ def git(*args):
event = event or {"before": "base"}
if event_name == "pull_request":
event = {"pull_request": {"number": 7, "head": {"sha": "head", "ref": "feature",
"repo": {"full_name": "alice/server"}}, "base": {"sha": "base"}}}
"repo": {"full_name": "alice/server"}}, "base": {"sha": "base", "ref": "master"}}}
with patch.object(policy, "git", side_effect=git), patch.object(
policy, "unsafe_documentation", return_value=False), patch.object(
policy, "require_current_pr"), patch.dict(os.environ, {"GITHUB_SHA": "merge"}):
Expand Down Expand Up @@ -283,19 +283,25 @@ def test_toolchain_selection_and_fixture_gate_contract_remain_compatible(self):

def test_toolchain_gate_keeps_summary_current_pr_and_cli_all(self):
plan = {"schema": 1, "repository": "apache/toolchain", "project": "toolchain", "pr": 7,
"source": "alice/toolchain", "branch": "feature", "base": "base", "head": "head",
"testedMergeSHA": "merge", "selected": ["client"], "expected": ["client"]}
"source": "alice/toolchain", "branch": "feature", "target": "master", "base": "base",
"head": "head", "testedMergeSHA": "merge", "selected": ["client"], "expected": ["client"]}
live = {"state": "open", "head": {"sha": "head", "ref": "feature", "repo": {"full_name": "alice/toolchain"}},
"base": {"repo": {"full_name": "apache/toolchain"}}}
"base": {"ref": "master", "repo": {"full_name": "apache/toolchain"}}}
def current(pr, status="ahead"):
return lambda path: {"status": status} if "/compare/" in path else pr
results = {"plan": {"result": "success"}, "client": {"result": "success"}, "fixture": {"result": "success"}}
with tempfile.TemporaryDirectory() as directory, patch("builtins.print"):
root = Path(directory)
summary = root / "summary.md"
with patch.dict(os.environ, {"GITHUB_STEP_SUMMARY": str(summary)}):
policy.gate(plan, results, lambda _: live)
policy.gate(plan, results, current(live))
self.assertIn("| client | yes | advisory | affected inputs | success |", summary.read_text())
with self.assertRaises(policy.StaleInputError):
policy.gate(plan, results, lambda _: dict(live, head=dict(live["head"], sha="new-head")))
policy.gate(plan, results, current(dict(live, head=dict(live["head"], sha="new-head"))))
with self.assertRaises(policy.StaleInputError):
policy.gate(plan, results, current(dict(live, base=dict(live["base"], ref="release"))))
with self.assertRaises(policy.StaleInputError):
policy.gate(plan, results, current(live, "diverged"))
(root / "plan.json").write_text(json.dumps(dict(plan, pr=0)))
(root / "results.json").write_text(json.dumps(results))
outcome = subprocess.run(["python3", str(Path(policy.__file__).resolve()), "gate", "--mode", "all",
Expand Down Expand Up @@ -654,23 +660,39 @@ def test_cumulative_pr_snapshot_keeps_source_after_document_commit_and_checks_cu
"-p", base, "-p", head, "-m", "PR merge")
fixture.git("checkout", "--detach", "-q", merge)
live = {"number": 7, "state": "open", "head": {"sha": head, "ref": "feature",
"repo": {"full_name": "alice/server"}}, "base": {"sha": base, "repo": {"full_name": "apache/server"}}}
"repo": {"full_name": "alice/server"}}, "base": {"sha": base, "ref": "master",
"repo": {"full_name": "apache/server"}}}
fixture.git("branch", "-f", "master", base)
calls = []
def fetch(path):
def fetch(path, pr=live):
calls.append(path)
return live
if "/compare/" not in path:
return pr
tested, target = path.rsplit("/", 1)[1].split("...")
if subprocess.run(["git", "merge-base", "--is-ancestor", tested, target], cwd=fixture.root).returncode:
return {"status": "diverged"}
return {"status": "identical" if fixture.git("rev-parse", target) == tested else "ahead"}
plan = fixture.plan({"pull_request": live}, fetch)
self.assertEqual(["repos/apache/server/pulls/7"], calls)
self.assertEqual(["repos/apache/server/pulls/7", "repos/apache/server/compare/" + base + "...master"],
calls)
self.assertTrue(plan["server"])
self.assertFalse(plan["pd_store"])
self.assertFalse(plan["security"])
self.assertIn("hugegraph-server/A.java", plan["changedPaths"])
for section, field, value in [("head", "sha", "new-head"), ("head", "ref", "other"),
("head", "repo", {"full_name": "other/fork"}),
("base", "repo", {"full_name": "other/target"})]:
("base", "repo", {"full_name": "other/target"}),
("base", "ref", "release")]:
changed = dict(live, **{section: dict(live[section], **{field: value})})
with self.subTest(section=section, field=field), self.assertRaises(policy.StaleInputError):
fixture.plan({"pull_request": live}, lambda _: changed)
fixture.plan({"pull_request": live}, lambda path: fetch(path, changed))
# Target advancement keeps the tested base; a force-push to unrelated history does not.
tree = fixture.git("rev-parse", base + "^{tree}")
fixture.git("branch", "-f", "master", fixture.git("commit-tree", tree, "-p", base, "-m", "advance"))
self.assertTrue(fixture.plan({"pull_request": live}, fetch)["server"])
fixture.git("branch", "-f", "master", fixture.git("commit-tree", tree, "-m", "unrelated"))
with self.assertRaises(policy.StaleInputError):
fixture.plan({"pull_request": live}, fetch)
with self.assertRaises(policy.StaleInputError):
fixture.plan({"pull_request": live}, lambda _: dict(live, state="closed"))
fixture.git("checkout", "--detach", "-q", head)
Expand All @@ -679,7 +701,10 @@ def fetch(path):

def test_queued_merge_uses_actual_target_parent_and_api_outage_keeps_pr_identity(self):
live = {"number": 7, "state": "open", "head": {"sha": "head", "ref": "feature",
"repo": {"full_name": "alice/server"}}, "base": {"sha": "event-base", "repo": {"full_name": "apache/server"}}}
"repo": {"full_name": "alice/server"}}, "base": {"sha": "event-base", "ref": "master",
"repo": {"full_name": "apache/server"}}}
def current(path):
return {"status": "ahead"} if "/compare/" in path else live
def git(*args):
if args[0] == "show":
return "current-base head"
Expand All @@ -688,7 +713,7 @@ def git(*args):
return "merge"
with patch.dict(os.environ, {"GITHUB_SHA": "merge", "GITHUB_EVENT_NAME": ""}), patch.object(
policy, "git", side_effect=git), patch.object(policy, "unsafe_documentation", return_value=False):
plan = policy.create_plan("server", {"pull_request": live}, "apache/server", lambda _: live)
plan = policy.create_plan("server", {"pull_request": live}, "apache/server", current)
self.assertEqual("current-base", plan["base"])
self.assertEqual([], plan["expected"])
def unavailable(_):
Expand Down
3 changes: 2 additions & 1 deletion docs/ci.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,8 @@ suite-selection and required-check policy.
PR selection uses the cumulative diff against the checked-out merge commit's base. A source
PR followed by a documentation-only commit still tests the cumulative source changes.
Preflight verifies that the event merge commit's second parent matches the current PR head.
A stale PR head fails preflight. Target-branch advancement alone does not invalidate a run.
A stale PR head, a changed target branch, or a target branch whose history no longer contains
the tested base fails preflight. Target-branch advancement alone does not invalidate a run.
Plans and selection summaries describe this run; previous results are never reused.

A new PR head or master push cancels the previous automatic run for that PR or master ref,
Expand Down
Loading