From 621f4fcdeeab171463216b9e9d285a0a8d353550 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 04:25:25 +0200 Subject: [PATCH] fix: give fork pull requests the hash report A fork's GITHUB_TOKEN is read-only, so the comment never arrived and a valid file turned red. The report now goes to the job summary for every pull request, and the comment and labels run only where the token can write. validate_pr points an oversized file to an issue instead of a platform setting a contributor cannot use. --- .github/workflows/validate.yml | 27 +++++++- scripts/validate_pr.py | 10 ++- tests/test_validate_pr_flow.py | 123 +++++++++++++++++++++++++++++++++ wiki/testing-guide.md | 2 +- 4 files changed, 157 insertions(+), 5 deletions(-) create mode 100644 tests/test_validate_pr_flow.py diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml index 94b4c64f..4e010a75 100644 --- a/.github/workflows/validate.yml +++ b/.github/workflows/validate.yml @@ -89,8 +89,24 @@ jobs: fi cat report.md - - name: Post validation report + # The job summary is the report every contributor sees, fork or not: + # it needs no write token. + - name: Write the report to the job summary if: always() + run: | + if [ -f report.md ]; then + cat report.md >> "$GITHUB_STEP_SUMMARY" + else + echo "The validation step stopped before writing a report." >> "$GITHUB_STEP_SUMMARY" + fi + + # A pull request from a fork runs with a read-only GITHUB_TOKEN, so a + # comment would fail and turn a valid file red. The comment is posted + # only when the branch lives in this repository. pull_request_target is + # not an option: it hands a write token to a run that would read the + # contributor's files and scripts. + - name: Post validation report + if: always() && github.event.pull_request.head.repo.full_name == github.repository run: | gh pr comment "${{ github.event.pull_request.number }}" --body-file report.md env: @@ -99,7 +115,7 @@ jobs: - name: Fail when a file did not validate if: steps.validate.outputs.rc != '0' run: | - echo "validate_pr.py exited ${{ steps.validate.outputs.rc }}" >&2 + echo "::error::validate_pr.py exited ${{ steps.validate.outputs.rc }}. The report is in the job summary of this run." exit 1 validate-configs: @@ -212,7 +228,11 @@ jobs: with: fetch-depth: 0 + # Labels need a write token, which a pull request from a fork does not + # get. The step is skipped there instead of reporting a success that + # set nothing. - name: Auto-label PR + if: github.event.pull_request.head.repo.full_name == github.repository run: | labels="" files=$(gh pr diff "${{ github.event.pull_request.number }}" --name-only) @@ -231,7 +251,8 @@ jobs: fi for label in $labels; do - gh pr edit "${{ github.event.pull_request.number }}" --add-label "$label" 2>/dev/null || true + gh pr edit "${{ github.event.pull_request.number }}" --add-label "$label" \ + || echo "::warning::label $label was not added" done env: GH_TOKEN: ${{ github.token }} diff --git a/scripts/validate_pr.py b/scripts/validate_pr.py index 451f1b0b..7c66ab88 100644 --- a/scripts/validate_pr.py +++ b/scripts/validate_pr.py @@ -61,6 +61,9 @@ BLOCKED_EXTENSIONS = { } MAX_FILE_SIZE = 100 * 1024 * 1024 +LARGE_FILE_ISSUE_URL = ( + "https://github.com/Abdess/retrobios/issues/new?template=missing-bios.yml" +) class ValidationResult: @@ -259,9 +262,14 @@ def validate_file( result.add_check(False, f"Blocked file extension: {ext}") if result.size > MAX_FILE_SIZE: + # GitHub refuses a pushed file over 100 MiB, so a pull request cannot + # carry it. Only a maintainer can add an asset to the release. result.add_check( False, - f"File too large for embedded storage ({result.size:,} > {MAX_FILE_SIZE:,} bytes). Use storage: external in platform config.", + f"File too large for git ({result.size:,} > {MAX_FILE_SIZE:,} bytes). " + "Leave it out of the pull request and open an issue " + f"({LARGE_FILE_ISSUE_URL}) with the hashes above and where the file " + "can be obtained: a maintainer adds it to the `large-files` release.", ) elif result.size == 0: result.add_check(False, "File is empty (0 bytes)") diff --git a/tests/test_validate_pr_flow.py b/tests/test_validate_pr_flow.py new file mode 100644 index 00000000..c6fb86c1 --- /dev/null +++ b/tests/test_validate_pr_flow.py @@ -0,0 +1,123 @@ +"""What a contributor gets back after adding a BIOS file. + +A pull request from a fork runs with a read-only token, so the hash report +has to reach the contributor without one. And the advice for a file git +cannot hold has to be an action the contributor can take. +""" + +from __future__ import annotations + +import os +import sys +import tempfile +import unittest +from pathlib import Path + +import yaml + +REPO_ROOT = Path(__file__).resolve().parent.parent +sys.path.insert(0, str(REPO_ROOT / "scripts")) + +import validate_pr # noqa: E402 + +WORKFLOW = REPO_ROOT / ".github" / "workflows" / "validate.yml" +SAME_REPO = "github.event.pull_request.head.repo.full_name == github.repository" + + +def _workflow() -> dict: + document = yaml.safe_load(WORKFLOW.read_text(encoding="utf-8")) + # PyYAML resolves the bare `on:` key to the boolean True. + document["on"] = document.pop(True, document.get("on")) + return document + + +def _steps(job: str) -> list[dict]: + return _workflow()["jobs"][job]["steps"] + + +class ForkPullRequestsGetTheReport(unittest.TestCase): + def test_the_report_goes_to_the_job_summary_whatever_the_outcome(self): + summary = [ + s for s in _steps("validate-bios") + if "GITHUB_STEP_SUMMARY" in str(s.get("run", "")) + ] + self.assertTrue(summary, "the report never reaches the job summary") + self.assertIn("report.md", str(summary[0]["run"])) + self.assertIn("always()", str(summary[0].get("if", ""))) + + def test_the_summary_is_written_before_the_failing_gate(self): + steps = _steps("validate-bios") + names = [str(s.get("run", "")) for s in steps] + summary = next(i for i, r in enumerate(names) if "GITHUB_STEP_SUMMARY" in r) + gate = next(i for i, s in enumerate(steps) if "rc != " in str(s.get("if", ""))) + self.assertLess(summary, gate) + + def test_the_comment_is_posted_only_where_the_token_can_write(self): + for step in _steps("validate-bios"): + if "gh pr comment" in str(step.get("run", "")): + self.assertIn(SAME_REPO, str(step.get("if", ""))) + + def test_labels_are_not_attempted_from_a_fork(self): + for step in _steps("label-pr"): + run = str(step.get("run", "")) + if "gh pr edit" in run: + self.assertIn(SAME_REPO, str(step.get("if", ""))) + self.assertNotIn("|| true", run, "a refused label vanishes in silence") + + def test_no_run_gets_a_write_token_for_fork_code(self): + triggers = _workflow()["on"] + self.assertNotIn("pull_request_target", triggers) + self.assertNotIn("workflow_run", triggers) + for path in (REPO_ROOT / ".github" / "workflows").glob("*.yml"): + data = yaml.safe_load(path.read_text(encoding="utf-8")) + # PyYAML reads the bare key `on` as True. + events = data.get("on", data.get(True)) or {} + self.assertNotIn("pull_request_target", events, path.name) + + def test_the_workflow_uses_no_third_party_action(self): + for job in _workflow()["jobs"].values(): + for step in job.get("steps", []): + uses = step.get("uses") + if uses: + self.assertTrue(uses.startswith("actions/"), uses) + + +class OversizedFileAdvice(unittest.TestCase): + def setUp(self): + self._tmp = tempfile.TemporaryDirectory() + self.tmp = Path(self._tmp.name) + self._cwd = os.getcwd() + os.chdir(self.tmp) + self.addCleanup(self._tmp.cleanup) + self.addCleanup(os.chdir, self._cwd) + self._max = validate_pr.MAX_FILE_SIZE + self.addCleanup(setattr, validate_pr, "MAX_FILE_SIZE", self._max) + + def _message(self) -> str: + path = self.tmp / "bios" / "Sony" / "PlayStation 3" / "PS3UPDAT.PUP" + path.parent.mkdir(parents=True) + path.write_bytes(b"PUP" * 4) + validate_pr.MAX_FILE_SIZE = 4 + result = validate_pr.validate_file( + str(path.relative_to(self.tmp)), None, + {"sha1": set(), "md5": set(), "names": set()}, + ) + self.assertFalse(result.passed) + return next(m for s, m in result.checks if s == "FAIL" and "too large" in m) + + def test_the_advice_is_an_issue_for_the_large_files_release(self): + message = self._message() + self.assertNotIn("storage: external", message) + self.assertIn("large-files", message) + self.assertIn("open an issue", message) + self.assertIn(validate_pr.LARGE_FILE_ISSUE_URL, message) + + def test_the_linked_issue_template_exists(self): + template = validate_pr.LARGE_FILE_ISSUE_URL.rsplit("template=", 1)[1] + self.assertTrue( + (REPO_ROOT / ".github" / "ISSUE_TEMPLATE" / template).is_file(), template + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/wiki/testing-guide.md b/wiki/testing-guide.md index bf4c3c1d..eba5c56c 100644 --- a/wiki/testing-guide.md +++ b/wiki/testing-guide.md @@ -46,7 +46,7 @@ Every module in `tests/`, by the area it guards. | Installer and downloads | loopback server | `test_check_release`, `test_download`, `test_install`, `test_install_cores`, `test_install_experience`, `test_install_selection` | | Truth and native export | synthetic + cached originals | `test_export_counts`, `test_export_native_layer`, `test_export_report`, `test_exporters`, `test_truth_diff`, `test_truth_layout` | | Scrapers, upstream and provenance | inline sources, synthetic | `test_check_freshness`, `test_dedup`, `test_fbneo_parser`, `test_find_pinned_revision`, `test_hash_merge`, `test_launchers`, `test_launchers_emudeck`, `test_launchers_recalbox`, `test_launchers_retrobat`, `test_launchers_retrodeck`, `test_launchers_retropie`, `test_launchers_rocknix`, `test_mame_devices`, `test_mame_parser`, `test_mame_ref_audit`, `test_profile_sync`, `test_provenance`, `test_refresh_data_dirs`, `test_refresh_failures`, `test_refresh_stale`, `test_retrodeck_scraper_errors`, `test_romset_recipes`, `test_scraper_contract`, `test_upstream`, `test_upstream_index` | -| Site, docs and repository contracts | rendered pages, sources | `test_contributing_text`, `test_data_dir_refs`, `test_dataclass_defaults`, `test_optional_dependencies`, `test_prose_style`, `test_site_availability`, `test_site_cores`, `test_site_exports`, `test_site_platform_hashes`, `test_site_system_links`, `test_site_validation`, `test_site_write`, `test_validate_pr`, `test_validate_pr_hashes`, `test_workflow_paths` | +| Site, docs and repository contracts | rendered pages, sources | `test_contributing_text`, `test_data_dir_refs`, `test_dataclass_defaults`, `test_optional_dependencies`, `test_prose_style`, `test_site_availability`, `test_site_cores`, `test_site_exports`, `test_site_platform_hashes`, `test_site_system_links`, `test_site_validation`, `test_site_write`, `test_validate_pr`, `test_validate_pr_flow`, `test_validate_pr_hashes`, `test_workflow_paths` | ## Test architecture