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

@@ -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 }}

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)")

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()

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