From f9de5af1a8636902ddd352fcf0e0918361c0c0f9 Mon Sep 17 00:00:00 2001 From: XuPeng-SH Date: Thu, 24 Sep 2026 20:49:48 +0800 Subject: [PATCH 1/4] ci: fall back to API for coverage PR diff --- .github/workflows/coverage-merge.yaml | 3 +- scripts/fetch_public_pr_diff.sh | 133 +++++++++++++++++++ scripts/test_fetch_public_pr_diff.py | 180 ++++++++++++++++++++++++++ 3 files changed, 315 insertions(+), 1 deletion(-) create mode 100644 scripts/fetch_public_pr_diff.sh create mode 100644 scripts/test_fetch_public_pr_diff.py diff --git a/.github/workflows/coverage-merge.yaml b/.github/workflows/coverage-merge.yaml index e263901..028a652 100644 --- a/.github/workflows/coverage-merge.yaml +++ b/.github/workflows/coverage-merge.yaml @@ -86,7 +86,8 @@ jobs: exit 1 fi if [ -n "${IS_PUB_REPO:-}" ] && [ "${IS_PUB_REPO}" != "0" ] && [ "${IS_PUB_REPO}" != "false" ]; then - curl -fL --retry 5 --retry-delay 3 "https://github.com/${pr_repo}/pull/${pr_number}.diff" -o diff.patch + timeout 240s bash "$GITHUB_WORKSPACE/CI/scripts/fetch_public_pr_diff.sh" \ + "$pr_repo" "$pr_number" "$EXPECTED_HEAD_SHA" diff.patch else curl -fL --retry 5 --retry-delay 3 \ -H 'Accept: application/vnd.github.v3.diff' \ diff --git a/scripts/fetch_public_pr_diff.sh b/scripts/fetch_public_pr_diff.sh new file mode 100644 index 0000000..df9a66a --- /dev/null +++ b/scripts/fetch_public_pr_diff.sh @@ -0,0 +1,133 @@ +#!/usr/bin/env bash + +set -euo pipefail + +usage() { + echo "usage: GH_TOKEN=... $0 " >&2 + exit 2 +} + +[[ $# -eq 4 ]] || usage + +pr_repo=$1 +pr_number=$2 +expected_head_sha=$3 +output_path=$4 +: "${GH_TOKEN:?GH_TOKEN is required for the authenticated API fallback}" + +[[ "$pr_repo" =~ ^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$ ]] || usage +[[ "$pr_number" =~ ^[1-9][0-9]*$ ]] || usage +[[ "$expected_head_sha" =~ ^[[:xdigit:]]{40}$ ]] || usage + +output_dir=$(dirname -- "$output_path") +mkdir -p -- "$output_dir" +# Never let a failed fetch leave a previous diff that the parser could consume. +rm -f -- "$output_path" +temp_dir=$(mktemp -d "${output_dir}/.fetch-pr-diff.XXXXXX") +trap 'rm -rf -- "$temp_dir"' EXIT + +api_pr_url="https://api.github.com/repos/${pr_repo}/pulls/${pr_number}" +candidate_patch="${temp_dir}/candidate.patch" +pr_metadata="${temp_dir}/pull-request.json" + +curl_with_retry=( + --silent --show-error --fail --location + --connect-timeout 5 --max-time 15 + --retry 4 --retry-delay 3 --retry-max-time 45 --retry-connrefused +) + +request() { + local source=$1 + local response_path=$2 + shift 2 + + local http_status="" + local curl_status=0 + http_status=$(curl "${curl_with_retry[@]}" "$@" \ + --output "$response_path" --write-out '%{http_code}') || curl_status=$? + if ((curl_status != 0)); then + echo "${source} request failed (curl exit ${curl_status}, HTTP ${http_status:-unknown})" >&2 + return 1 + fi + if [[ "$http_status" != "200" ]]; then + echo "${source} request returned unexpected HTTP ${http_status:-unknown}" >&2 + return 1 + fi +} + +current_pr_head() { + request "GitHub PR metadata" "$pr_metadata" \ + -H 'Accept: application/vnd.github+json' \ + -H "Authorization: Bearer ${GH_TOKEN}" \ + -H 'X-GitHub-Api-Version: 2022-11-28' \ + "$api_pr_url" || return 1 + + local head_sha + head_sha=$(jq -er '.head.sha | select(type == "string")' "$pr_metadata") || { + echo "GitHub PR metadata did not contain a head SHA" >&2 + return 1 + } + if [[ ! "$head_sha" =~ ^[[:xdigit:]]{40}$ ]]; then + echo "GitHub PR metadata returned an invalid head SHA" >&2 + return 1 + fi + printf '%s' "$head_sha" +} + +verify_pr_head() { + local phase=$1 + local current_head_sha + current_head_sha=$(current_pr_head) || return 1 + if [[ "$current_head_sha" != "$expected_head_sha" ]]; then + echo "PR head moved ${phase}: expected ${expected_head_sha}, found ${current_head_sha}; refusing to publish coverage diff" >&2 + return 1 + fi +} + +validate_diff() { + local source=$1 + local first_line + if [[ ! -s "$candidate_patch" ]]; then + # A successful empty diff is valid and means there are no changed lines. + return 0 + fi + + IFS= read -r first_line < "$candidate_patch" || true + if [[ "$first_line" != 'diff --git '* ]]; then + echo "${source} returned a non-empty response that is not a Git diff; refusing to treat it as no changed lines" >&2 + return 1 + fi +} + +download_diff() { + local source=$1 + shift + : > "$candidate_patch" + if ! request "$source" "$candidate_patch" "$@"; then + return 1 + fi + validate_diff "$source" +} + +verify_pr_head "before diff download" + +public_diff_url="https://github.com/${pr_repo}/pull/${pr_number}.diff" +if download_diff "public PR diff" "$public_diff_url"; then + echo "Downloaded PR diff from public URL" +else + echo "Public PR diff failed; falling back to authenticated GitHub API" >&2 + if ! download_diff "GitHub API PR diff" \ + -H 'Accept: application/vnd.github.diff' \ + -H "Authorization: Bearer ${GH_TOKEN}" \ + -H 'X-GitHub-Api-Version: 2022-11-28' \ + "$api_pr_url"; then + echo "Unable to download a valid PR diff from either GitHub endpoint" >&2 + exit 1 + fi + echo "Downloaded PR diff from authenticated GitHub API" +fi + +# The PR can be updated while the diff endpoints are retrying. Publish only +# when both metadata checks bracket the download at the expected immutable SHA. +verify_pr_head "after diff download" +mv -- "$candidate_patch" "$output_path" diff --git a/scripts/test_fetch_public_pr_diff.py b/scripts/test_fetch_public_pr_diff.py new file mode 100644 index 0000000..9521904 --- /dev/null +++ b/scripts/test_fetch_public_pr_diff.py @@ -0,0 +1,180 @@ +import json +import os +import subprocess +import tempfile +import unittest +from pathlib import Path + + +EXPECTED_SHA = "a" * 40 +MOVED_SHA = "b" * 40 +VALID_DIFF = b"diff --git a/main.go b/main.go\n--- a/main.go\n+++ b/main.go\n@@ -1 +1 @@\n-old\n+new\n" + + +FAKE_CURL = r'''#!/usr/bin/env python3 +import json +import os +import sys +from pathlib import Path + +args = sys.argv[1:] +url = next(arg for arg in args if arg.startswith("https://")) +output = Path(args[args.index("--output") + 1]) +has_auth = any( + args[i + 1] == "Authorization: Bearer " + os.environ["GH_TOKEN"] + for i, arg in enumerate(args[:-1]) + if arg == "-H" +) +mode = os.environ["FAKE_CURL_MODE"] +state_path = Path(os.environ["FAKE_CURL_STATE"]) +try: + state = json.loads(state_path.read_text()) +except FileNotFoundError: + state = {"heads": 0, "calls": []} + +api_diff = any("application/vnd.github.diff" in arg for arg in args) +if "/pulls/42" in url and "api.github.com" in url and not api_diff: + state["heads"] += 1 + if mode == "head-moves" and state["heads"] > 1: + body = json.dumps({"head": {"sha": os.environ["MOVED_SHA"]}}).encode() + else: + body = json.dumps({"head": {"sha": os.environ["EXPECTED_SHA"]}}).encode() + status = 200 + kind = "metadata" +elif "github.com/owner/repo/pull/42.diff" in url: + kind = "public" + if mode in ("public-503-api-ok", "api-fails"): + status, body = 503, b"partial service error" + elif mode == "public-empty": + status, body = 200, b"" + elif mode == "public-html": + status, body = 200, b"temporary failure" + elif mode in ("public-ok", "head-moves"): + status, body = 200, os.environ["VALID_DIFF"].encode() + else: + status, body = 500, b"unexpected mode" +elif "api.github.com/repos/owner/repo/pulls/42" in url and api_diff: + kind = "api-diff" + if mode == "api-fails": + status, body = 403, b"forbidden" + else: + status, body = 200, os.environ["VALID_DIFF"].encode() +else: + kind = "unexpected" + status, body = 500, b"unexpected URL" + +state["calls"].append({ + "kind": kind, + "authenticated": has_auth, + "retry_count": args[args.index("--retry") + 1], + "has_timeout": "--max-time" in args, +}) +state_path.write_text(json.dumps(state)) +output.write_bytes(body) +sys.stdout.write(str(status)) +if status >= 400: + sys.exit(22) +''' + + +class FetchPublicPRDiffTest(unittest.TestCase): + def setUp(self): + self.temp = tempfile.TemporaryDirectory() + self.root = Path(self.temp.name) + bin_dir = self.root / "bin" + bin_dir.mkdir() + fake_curl = bin_dir / "curl" + fake_curl.write_text(FAKE_CURL) + fake_curl.chmod(0o755) + self.state_path = self.root / "curl-state.json" + self.output = self.root / "matrixone" / "diff.patch" + self.env = os.environ.copy() + self.env.update( + { + "PATH": f"{bin_dir}:{self.env['PATH']}", + "GH_TOKEN": "test-token-must-not-be-logged", + "FAKE_CURL_STATE": str(self.state_path), + "EXPECTED_SHA": EXPECTED_SHA, + "MOVED_SHA": MOVED_SHA, + "VALID_DIFF": VALID_DIFF.decode(), + } + ) + + def tearDown(self): + self.temp.cleanup() + + def run_fetch(self, mode): + self.env["FAKE_CURL_MODE"] = mode + return subprocess.run( + [ + "bash", + str(Path(__file__).with_name("fetch_public_pr_diff.sh")), + "owner/repo", + "42", + EXPECTED_SHA, + str(self.output), + ], + env=self.env, + check=False, + capture_output=True, + text=True, + timeout=10, + ) + + def read_state(self): + return json.loads(self.state_path.read_text()) + + def test_public_503_falls_back_to_api_and_checks_head_twice(self): + result = self.run_fetch("public-503-api-ok") + + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(self.output.read_bytes(), VALID_DIFF) + state = self.read_state() + self.assertEqual(state["heads"], 2) + self.assertEqual([call["kind"] for call in state["calls"]], [ + "metadata", "public", "api-diff", "metadata" + ]) + self.assertFalse(state["calls"][1]["authenticated"]) + self.assertTrue(state["calls"][2]["authenticated"]) + self.assertNotIn(self.env["GH_TOKEN"], result.stdout + result.stderr) + self.assertEqual(state["calls"][1]["retry_count"], "4") + self.assertTrue(all(call["has_timeout"] for call in state["calls"])) + + def test_api_failure_does_not_publish_partial_diff(self): + self.output.parent.mkdir(parents=True) + self.output.write_bytes(b"stale diff from an earlier attempt") + result = self.run_fetch("api-fails") + + self.assertNotEqual(result.returncode, 0) + self.assertFalse(self.output.exists()) + self.assertIn("HTTP 403", result.stderr) + self.assertEqual(list(self.output.parent.glob(".fetch-pr-diff.*")), []) + + def test_head_change_during_download_does_not_publish_diff(self): + result = self.run_fetch("head-moves") + + self.assertNotEqual(result.returncode, 0) + self.assertFalse(self.output.exists()) + self.assertIn("PR head moved after diff download", result.stderr) + + def test_empty_successful_diff_is_preserved_as_empty(self): + result = self.run_fetch("public-empty") + + self.assertEqual(result.returncode, 0, result.stderr) + self.assertTrue(self.output.exists()) + self.assertEqual(self.output.read_bytes(), b"") + self.assertEqual([call["kind"] for call in self.read_state()["calls"]], [ + "metadata", "public", "metadata" + ]) + + def test_non_diff_http_200_body_falls_back_instead_of_becoming_no_changes(self): + result = self.run_fetch("public-html") + + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(self.output.read_bytes(), VALID_DIFF) + self.assertIn("not a Git diff", result.stderr) + self.assertIn("api-diff", [call["kind"] for call in self.read_state()["calls"]]) + + +if __name__ == "__main__": + unittest.main() From 21a25fb69662ed2ccbeb40efc43ceff0fb9ce7fc Mon Sep 17 00:00:00 2001 From: XuPeng-SH Date: Thu, 24 Sep 2026 12:48:55 -0400 Subject: [PATCH 2/4] ci(coverage): reject empty diffs for changed PRs --- scripts/fetch_public_pr_diff.sh | 26 ++++++++++++-- scripts/test_fetch_public_pr_diff.py | 52 ++++++++++++++++++++++++---- 2 files changed, 69 insertions(+), 9 deletions(-) diff --git a/scripts/fetch_public_pr_diff.sh b/scripts/fetch_public_pr_diff.sh index df9a66a..1619ef4 100644 --- a/scripts/fetch_public_pr_diff.sh +++ b/scripts/fetch_public_pr_diff.sh @@ -84,13 +84,29 @@ verify_pr_head() { fi } +pr_changed_files() { + local changed_files + changed_files=$(jq -er '.changed_files | select(type == "number" and . >= 0 and . == floor)' "$pr_metadata") || { + echo "GitHub PR metadata did not contain a valid changed-files count" >&2 + return 1 + } + printf '%s' "$changed_files" +} + validate_diff() { local source=$1 local first_line if [[ ! -s "$candidate_patch" ]]; then - # A successful empty diff is valid and means there are no changed lines. + if [[ "$expected_changed_files" != "0" ]]; then + echo "${source} returned an empty diff despite changed_files=${expected_changed_files}" >&2 + return 1 + fi return 0 fi + if [[ "$expected_changed_files" == "0" ]]; then + echo "${source} returned a non-empty diff despite changed_files=0" >&2 + return 1 + fi IFS= read -r first_line < "$candidate_patch" || true if [[ "$first_line" != 'diff --git '* ]]; then @@ -110,6 +126,7 @@ download_diff() { } verify_pr_head "before diff download" +expected_changed_files=$(pr_changed_files) public_diff_url="https://github.com/${pr_repo}/pull/${pr_number}.diff" if download_diff "public PR diff" "$public_diff_url"; then @@ -128,6 +145,11 @@ else fi # The PR can be updated while the diff endpoints are retrying. Publish only -# when both metadata checks bracket the download at the expected immutable SHA. +# when both metadata checks agree on the expected head and changed-file count. verify_pr_head "after diff download" +current_changed_files=$(pr_changed_files) +if [[ "$current_changed_files" != "$expected_changed_files" ]]; then + echo "PR changed-file count moved during diff download: expected ${expected_changed_files}, found ${current_changed_files}; refusing to publish coverage diff" >&2 + exit 1 +fi mv -- "$candidate_patch" "$output_path" diff --git a/scripts/test_fetch_public_pr_diff.py b/scripts/test_fetch_public_pr_diff.py index 9521904..2ad1550 100644 --- a/scripts/test_fetch_public_pr_diff.py +++ b/scripts/test_fetch_public_pr_diff.py @@ -35,21 +35,26 @@ api_diff = any("application/vnd.github.diff" in arg for arg in args) if "/pulls/42" in url and "api.github.com" in url and not api_diff: state["heads"] += 1 - if mode == "head-moves" and state["heads"] > 1: - body = json.dumps({"head": {"sha": os.environ["MOVED_SHA"]}}).encode() - else: - body = json.dumps({"head": {"sha": os.environ["EXPECTED_SHA"]}}).encode() + head_sha = ( + os.environ["MOVED_SHA"] + if mode == "head-moves" and state["heads"] > 1 + else os.environ["EXPECTED_SHA"] + ) + changed_files = 0 if mode in ("public-empty", "public-nonempty-metadata-empty") else 1 + if mode == "changed-files-move" and state["heads"] > 1: + changed_files = 2 + body = json.dumps({"head": {"sha": head_sha}, "changed_files": changed_files}).encode() status = 200 kind = "metadata" elif "github.com/owner/repo/pull/42.diff" in url: kind = "public" if mode in ("public-503-api-ok", "api-fails"): status, body = 503, b"partial service error" - elif mode == "public-empty": + elif mode in ("public-empty", "public-empty-api-ok", "both-empty"): status, body = 200, b"" elif mode == "public-html": status, body = 200, b"temporary failure" - elif mode in ("public-ok", "head-moves"): + elif mode in ("public-ok", "head-moves", "changed-files-move", "public-nonempty-metadata-empty"): status, body = 200, os.environ["VALID_DIFF"].encode() else: status, body = 500, b"unexpected mode" @@ -57,6 +62,8 @@ kind = "api-diff" if mode == "api-fails": status, body = 403, b"forbidden" + elif mode == "both-empty": + status, body = 200, b"" else: status, body = 200, os.environ["VALID_DIFF"].encode() else: @@ -157,7 +164,7 @@ def test_head_change_during_download_does_not_publish_diff(self): self.assertFalse(self.output.exists()) self.assertIn("PR head moved after diff download", result.stderr) - def test_empty_successful_diff_is_preserved_as_empty(self): + def test_empty_diff_for_zero_changed_files_is_preserved(self): result = self.run_fetch("public-empty") self.assertEqual(result.returncode, 0, result.stderr) @@ -167,6 +174,37 @@ def test_empty_successful_diff_is_preserved_as_empty(self): "metadata", "public", "metadata" ]) + def test_empty_public_diff_with_changed_files_falls_back_to_api(self): + result = self.run_fetch("public-empty-api-ok") + + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(self.output.read_bytes(), VALID_DIFF) + self.assertIn("empty diff despite changed_files=1", result.stderr) + self.assertEqual([call["kind"] for call in self.read_state()["calls"]], [ + "metadata", "public", "api-diff", "metadata" + ]) + + def test_empty_diff_from_both_sources_does_not_pass_coverage(self): + result = self.run_fetch("both-empty") + + self.assertNotEqual(result.returncode, 0) + self.assertFalse(self.output.exists()) + self.assertIn("Unable to download a valid PR diff", result.stderr) + + def test_nonempty_diff_with_zero_changed_files_is_rejected(self): + result = self.run_fetch("public-nonempty-metadata-empty") + + self.assertNotEqual(result.returncode, 0) + self.assertFalse(self.output.exists()) + self.assertIn("non-empty diff despite changed_files=0", result.stderr) + + def test_changed_file_count_moving_during_download_does_not_publish_diff(self): + result = self.run_fetch("changed-files-move") + + self.assertNotEqual(result.returncode, 0) + self.assertFalse(self.output.exists()) + self.assertIn("PR changed-file count moved during diff download", result.stderr) + def test_non_diff_http_200_body_falls_back_instead_of_becoming_no_changes(self): result = self.run_fetch("public-html") From 984c329a30bccad73cf202f1c25c767655b49d8e Mon Sep 17 00:00:00 2001 From: XuPeng-SH Date: Thu, 24 Sep 2026 12:58:33 -0400 Subject: [PATCH 3/4] ci(coverage): validate PR diff structure and file count --- .github/workflows/check-action-file.yaml | 2 ++ scripts/fetch_public_pr_diff.sh | 10 ++++++++++ scripts/test_fetch_public_pr_diff.py | 21 ++++++++++++++++++++- 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/.github/workflows/check-action-file.yaml b/.github/workflows/check-action-file.yaml index bce21bd..9bb7c01 100644 --- a/.github/workflows/check-action-file.yaml +++ b/.github/workflows/check-action-file.yaml @@ -63,6 +63,8 @@ jobs: run: ${{ steps.install-actionlint.outputs.executable }} -shellcheck= -pyflakes= -color - name: Test coverage artifact generation selection run: python3 scripts/test_select_coverage_artifacts.py -v + - name: Test coverage PR diff fallback + run: python3 scripts/test_fetch_public_pr_diff.py -v - name: Test TKE merge subject identity contract run: python3 scripts/test_merge_trigger_tke_subject.py -v - name: Test SCA license scope selection diff --git a/scripts/fetch_public_pr_diff.sh b/scripts/fetch_public_pr_diff.sh index 1619ef4..b73d535 100644 --- a/scripts/fetch_public_pr_diff.sh +++ b/scripts/fetch_public_pr_diff.sh @@ -113,6 +113,16 @@ validate_diff() { echo "${source} returned a non-empty response that is not a Git diff; refusing to treat it as no changed lines" >&2 return 1 fi + + local diff_file_count + if ! diff_file_count=$(git apply --numstat -- "$candidate_patch" | awk 'END { print NR }'); then + echo "${source} returned a malformed Git diff" >&2 + return 1 + fi + if [[ "$diff_file_count" != "$expected_changed_files" ]]; then + echo "${source} diff has file_count=${diff_file_count} despite changed_files=${expected_changed_files}" >&2 + return 1 + fi } download_diff() { diff --git a/scripts/test_fetch_public_pr_diff.py b/scripts/test_fetch_public_pr_diff.py index 2ad1550..91453fc 100644 --- a/scripts/test_fetch_public_pr_diff.py +++ b/scripts/test_fetch_public_pr_diff.py @@ -43,6 +43,8 @@ changed_files = 0 if mode in ("public-empty", "public-nonempty-metadata-empty") else 1 if mode == "changed-files-move" and state["heads"] > 1: changed_files = 2 + if mode == "metadata-two-files": + changed_files = 2 body = json.dumps({"head": {"sha": head_sha}, "changed_files": changed_files}).encode() status = 200 kind = "metadata" @@ -54,7 +56,9 @@ status, body = 200, b"" elif mode == "public-html": status, body = 200, b"temporary failure" - elif mode in ("public-ok", "head-moves", "changed-files-move", "public-nonempty-metadata-empty"): + elif mode == "public-truncated": + status, body = 200, b"diff --git a/main.go b/main.go\n" + elif mode in ("public-ok", "head-moves", "changed-files-move", "public-nonempty-metadata-empty", "metadata-two-files"): status, body = 200, os.environ["VALID_DIFF"].encode() else: status, body = 500, b"unexpected mode" @@ -213,6 +217,21 @@ def test_non_diff_http_200_body_falls_back_instead_of_becoming_no_changes(self): self.assertIn("not a Git diff", result.stderr) self.assertIn("api-diff", [call["kind"] for call in self.read_state()["calls"]]) + def test_truncated_http_200_diff_falls_back_to_api(self): + result = self.run_fetch("public-truncated") + + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(self.output.read_bytes(), VALID_DIFF) + self.assertIn("malformed Git diff", result.stderr) + self.assertIn("api-diff", [call["kind"] for call in self.read_state()["calls"]]) + + def test_diff_with_fewer_files_than_metadata_is_rejected(self): + result = self.run_fetch("metadata-two-files") + + self.assertNotEqual(result.returncode, 0) + self.assertFalse(self.output.exists()) + self.assertIn("diff has file_count=1 despite changed_files=2", result.stderr) + if __name__ == "__main__": unittest.main() From 00b4a364df68e3b4a2613f9f98e26812d25ec4c2 Mon Sep 17 00:00:00 2001 From: XuPeng-SH Date: Thu, 24 Sep 2026 13:06:28 -0400 Subject: [PATCH 4/4] ci(coverage): match diff line counts to PR metadata --- scripts/fetch_public_pr_diff.sh | 32 ++++++++++++++----------- scripts/test_fetch_public_pr_diff.py | 35 ++++++++++++++++++++++++---- 2 files changed, 49 insertions(+), 18 deletions(-) diff --git a/scripts/fetch_public_pr_diff.sh b/scripts/fetch_public_pr_diff.sh index b73d535..6fc1ca2 100644 --- a/scripts/fetch_public_pr_diff.sh +++ b/scripts/fetch_public_pr_diff.sh @@ -84,13 +84,13 @@ verify_pr_head() { fi } -pr_changed_files() { - local changed_files - changed_files=$(jq -er '.changed_files | select(type == "number" and . >= 0 and . == floor)' "$pr_metadata") || { - echo "GitHub PR metadata did not contain a valid changed-files count" >&2 +pr_diff_stats() { + local stats + stats=$(jq -er '[.changed_files, .additions, .deletions] | select(all(.[]; type == "number" and . >= 0 and . == floor)) | @tsv' "$pr_metadata") || { + echo "GitHub PR metadata did not contain valid diff statistics" >&2 return 1 } - printf '%s' "$changed_files" + printf '%s' "$stats" } validate_diff() { @@ -114,13 +114,16 @@ validate_diff() { return 1 fi - local diff_file_count - if ! diff_file_count=$(git apply --numstat -- "$candidate_patch" | awk 'END { print NR }'); then + local diff_stats + if ! diff_stats=$(git apply --numstat -- "$candidate_patch" | awk -F '\t' ' + { files++; if ($1 != "-") additions += $1; if ($2 != "-") deletions += $2 } + END { printf "%d\t%d\t%d", files, additions, deletions } + '); then echo "${source} returned a malformed Git diff" >&2 return 1 fi - if [[ "$diff_file_count" != "$expected_changed_files" ]]; then - echo "${source} diff has file_count=${diff_file_count} despite changed_files=${expected_changed_files}" >&2 + if [[ "$diff_stats" != "$expected_stats" ]]; then + echo "${source} diff stats (${diff_stats}) differ from PR metadata (${expected_stats})" >&2 return 1 fi } @@ -136,7 +139,8 @@ download_diff() { } verify_pr_head "before diff download" -expected_changed_files=$(pr_changed_files) +expected_stats=$(pr_diff_stats) +IFS=$'\t' read -r expected_changed_files expected_additions expected_deletions <<< "$expected_stats" public_diff_url="https://github.com/${pr_repo}/pull/${pr_number}.diff" if download_diff "public PR diff" "$public_diff_url"; then @@ -155,11 +159,11 @@ else fi # The PR can be updated while the diff endpoints are retrying. Publish only -# when both metadata checks agree on the expected head and changed-file count. +# when both metadata checks agree on the expected head and diff statistics. verify_pr_head "after diff download" -current_changed_files=$(pr_changed_files) -if [[ "$current_changed_files" != "$expected_changed_files" ]]; then - echo "PR changed-file count moved during diff download: expected ${expected_changed_files}, found ${current_changed_files}; refusing to publish coverage diff" >&2 +current_stats=$(pr_diff_stats) +if [[ "$current_stats" != "$expected_stats" ]]; then + echo "PR diff statistics moved during diff download: expected (${expected_stats}), found (${current_stats}); refusing to publish coverage diff" >&2 exit 1 fi mv -- "$candidate_patch" "$output_path" diff --git a/scripts/test_fetch_public_pr_diff.py b/scripts/test_fetch_public_pr_diff.py index 91453fc..9cbbe63 100644 --- a/scripts/test_fetch_public_pr_diff.py +++ b/scripts/test_fetch_public_pr_diff.py @@ -9,6 +9,7 @@ EXPECTED_SHA = "a" * 40 MOVED_SHA = "b" * 40 VALID_DIFF = b"diff --git a/main.go b/main.go\n--- a/main.go\n+++ b/main.go\n@@ -1 +1 @@\n-old\n+new\n" +TWO_HUNK_DIFF = VALID_DIFF + b"@@ -3 +3 @@\n-old2\n+new2\n" FAKE_CURL = r'''#!/usr/bin/env python3 @@ -45,7 +46,15 @@ changed_files = 2 if mode == "metadata-two-files": changed_files = 2 - body = json.dumps({"head": {"sha": head_sha}, "changed_files": changed_files}).encode() + line_changes = 0 if changed_files == 0 else 1 + if mode in ("short-hunk-recovery", "short-hunk-both"): + line_changes = 2 + body = json.dumps({ + "head": {"sha": head_sha}, + "changed_files": changed_files, + "additions": line_changes, + "deletions": line_changes, + }).encode() status = 200 kind = "metadata" elif "github.com/owner/repo/pull/42.diff" in url: @@ -58,7 +67,7 @@ status, body = 200, b"temporary failure" elif mode == "public-truncated": status, body = 200, b"diff --git a/main.go b/main.go\n" - elif mode in ("public-ok", "head-moves", "changed-files-move", "public-nonempty-metadata-empty", "metadata-two-files"): + elif mode in ("public-ok", "head-moves", "changed-files-move", "public-nonempty-metadata-empty", "metadata-two-files", "short-hunk-recovery", "short-hunk-both"): status, body = 200, os.environ["VALID_DIFF"].encode() else: status, body = 500, b"unexpected mode" @@ -68,6 +77,8 @@ status, body = 403, b"forbidden" elif mode == "both-empty": status, body = 200, b"" + elif mode == "short-hunk-recovery": + status, body = 200, os.environ["TWO_HUNK_DIFF"].encode() else: status, body = 200, os.environ["VALID_DIFF"].encode() else: @@ -108,6 +119,7 @@ def setUp(self): "EXPECTED_SHA": EXPECTED_SHA, "MOVED_SHA": MOVED_SHA, "VALID_DIFF": VALID_DIFF.decode(), + "TWO_HUNK_DIFF": TWO_HUNK_DIFF.decode(), } ) @@ -207,7 +219,7 @@ def test_changed_file_count_moving_during_download_does_not_publish_diff(self): self.assertNotEqual(result.returncode, 0) self.assertFalse(self.output.exists()) - self.assertIn("PR changed-file count moved during diff download", result.stderr) + self.assertIn("PR diff statistics moved during diff download", result.stderr) def test_non_diff_http_200_body_falls_back_instead_of_becoming_no_changes(self): result = self.run_fetch("public-html") @@ -230,7 +242,22 @@ def test_diff_with_fewer_files_than_metadata_is_rejected(self): self.assertNotEqual(result.returncode, 0) self.assertFalse(self.output.exists()) - self.assertIn("diff has file_count=1 despite changed_files=2", result.stderr) + self.assertIn("diff stats", result.stderr) + + def test_partial_hunk_diff_falls_back_to_api(self): + result = self.run_fetch("short-hunk-recovery") + + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(self.output.read_bytes(), TWO_HUNK_DIFF) + self.assertIn("diff stats", result.stderr) + self.assertIn("api-diff", [call["kind"] for call in self.read_state()["calls"]]) + + def test_partial_hunk_diff_from_both_sources_is_rejected(self): + result = self.run_fetch("short-hunk-both") + + self.assertNotEqual(result.returncode, 0) + self.assertFalse(self.output.exists()) + self.assertIn("Unable to download a valid PR diff", result.stderr) if __name__ == "__main__":