fix: mark held and unknown PR files for review

validate_pr said all checks passed for random bytes and for a copy of a held file, and sent the copy to the folder that already holds it. Each now reads REVIEW with its next step, the status line counts them, and a held copy gets no placement hint. A copy inside a tree kept out of dedup is still accepted.

scripts/validate_pr.py

@@ -36,6 +36,7 @@ from common import (
 )
 from hashing import parse_hash_list
 from nativemode import digest_algorithm
+from dedup import _in_nodedup_dir
 from validation import frontend_digest_matches, inner_rom_check
 
 try:
@@ -79,6 +80,10 @@ class ValidationResult:
         self.md5 = ""
         self.crc32 = ""
         self.size = 0
+        # "held" (a copy of a file the collection has) or "unknown" (no list
+        # or profile declares it): no check fails, and neither is a file
+        # the pull request adds to the collection.
+        self.outcome = ""
 
     def add_check(self, passed: bool, message: str) -> None:
         self.checks.append(("PASS" if passed else "FAIL", message))
@@ -93,8 +98,12 @@ class ValidationResult:
     def passed(self) -> bool:
         return all(s != "FAIL" for s, _ in self.checks)
 
+    @property
+    def status(self) -> str:
+        return "FAIL" if not self.passed else "REVIEW" if self.outcome else "PASS"
+
     def to_markdown(self) -> str:
-        status = "PASS" if self.passed else "FAIL"
+        status = self.status
         lines = [f"### {status} `{self.filename}`"]
         lines.append("")
         lines.append(f"- **Path**: `{self.filepath}`")
@@ -374,9 +383,15 @@ def validate_file(
                 # PR that regenerated database.json was told each new file
                 # duplicated itself.
                 result.add_check(True, "Already in the database at this path")
+            elif _in_nodedup_dir(relative):
+                result.add_info(
+                    f"Identical to `{existing['path']}`, kept: this tree is read by path"
+                )
             else:
+                result.outcome = "held"
                 result.add_warning(
-                    f"Duplicate: identical file already exists at `{existing['path']}`"
+                    f"Already held at `{existing['path']}`: nothing to add, "
+                    "leave this copy out of the pull request"
                 )
         else:
             result.add_check(True, "Not a duplicate in database")
@@ -421,8 +436,11 @@ def validate_file(
             "Filename matches a known requirement but hash differs - may be a variant"
         )
     else:
+        result.outcome = "unknown"
         result.add_warning(
-            "File not referenced in any platform config - needs manual review"
+            "Not recognised: no platform list or emulator profile declares this "
+            "file. Say in the pull request which emulator reads it, for which "
+            "system, and where the dump comes from"
         )
 
     normalized = os.path.normpath(relative)
@@ -452,6 +470,8 @@ def validate_file(
             result.add_check(True, f"Correct placement: bios/{parts[1]}/{parts[2]}/")
         else:
             result.add_warning("File should be in bios/Manufacturer/Console/ structure")
+    elif result.outcome == "held":
+        pass
     elif expected_folders:
         # The contributor asks before forking: say where it goes.
         result.add_warning(
@@ -750,6 +770,7 @@ def main() -> None:
                 {
                     "file": r.filepath,
                     "passed": r.passed,
+                    "outcome": r.outcome or "new",
                     "sha1": r.sha1,
                     "md5": r.md5,
                     "size": r.size,
@@ -759,7 +780,15 @@ def main() -> None:
         print(json.dumps(output, indent=2))
     elif args.markdown:
         lines = ["## BIOS Validation Report", ""]
-        status = "all checks passed" if all_passed else "some checks failed"
+        held = sum(r.outcome == "held" for r in results)
+        unknown = sum(r.outcome == "unknown" for r in results)
+        review = ", ".join(part for part in (
+            f"{held} already held" if held else "",
+            f"{unknown} not recognised" if unknown else "",
+        ) if part)
+        status = ("some checks failed" if not all_passed
+                  else f"no check failed, to review: {review}" if review
+                  else "all checks passed")
         lines.append(f"**Status**: {status}")
         lines.append("")
 
@@ -770,8 +799,7 @@ def main() -> None:
         print("\n".join(lines))
     else:
         for r in results:
-            status = "PASS" if r.passed else "FAIL"
-            print(f"\n[{status}] {r.filepath}")
+            print(f"\n[{r.status}] {r.filepath}")
             if r.sha1:
                 print(f"  SHA1: {r.sha1}")
                 print(f"  MD5:  {r.md5}")

tests/test_validate_pr.py

@@ -243,20 +243,38 @@ class HashRecognition(_Fixture):
         self.assertIn(".variants/known.bin.", markdown)
 
     def test_an_unreferenced_file_asks_for_review(self):
+        """Random bytes read "PASS" and "all checks passed", and the warning
+        named the platform configs alone though the profiles were searched."""
         rel = self._bios_file("bios/Sony/PS/mystery.bin")
-        markdown = validate_pr.validate_file(rel, None, EMPTY_HASHES).to_markdown()
-        self.assertIn("needs manual review", markdown)
+        result = validate_pr.validate_file(rel, None, EMPTY_HASHES)
+        markdown = result.to_markdown()
+        self.assertEqual(result.outcome, "unknown")
+        self.assertIn("### REVIEW", markdown)
+        self.assertIn("no platform list or emulator profile declares this file", markdown)
+        self.assertIn("which emulator reads it", markdown)
 
     def test_a_duplicate_of_a_known_file_is_reported(self):
-        rel = self._bios_file("bios/Sony/PS/dupe.bin")
+        """A copy of a held file passed, and was sent to the folder that
+        already holds it."""
+        rel = self._bios_file("elsewhere/dupe.bin")
         probe = validate_pr.validate_file(rel, None, EMPTY_HASHES)
         db = {"files": {probe.sha1: {"path": "bios/Sony/PS/original.bin"}}}
-        markdown = validate_pr.validate_file(rel, db, EMPTY_HASHES).to_markdown()
-        self.assertIn("Duplicate", markdown)
-        self.assertIn("original.bin", markdown)
+        known = {**EMPTY_HASHES, "sha1": {probe.sha1},
+                 "folders": {probe.sha1: {"bios/Sony/PS/"}}}
+        result = validate_pr.validate_file(rel, db, known)
+        markdown = result.to_markdown()
+        self.assertEqual(result.outcome, "held")
+        self.assertIn("### REVIEW", markdown)
+        self.assertIn("Already held at `bios/Sony/PS/original.bin`: nothing to add", markdown)
+        self.assertNotIn("not under bios/", markdown)
         # A duplicate is worth saying out loud but must not block the PR.
-        self.assertTrue(validate_pr.validate_file(rel, db, EMPTY_HASHES).passed)
+        self.assertTrue(result.passed)
 
+    def test_a_copy_in_a_tree_read_by_path_is_kept(self):
+        rel = self._bios_file("bios/ScummVM/a/dupe.bin")
+        probe = validate_pr.validate_file(rel, None, EMPTY_HASHES)
+        db = {"files": {probe.sha1: {"path": "bios/ScummVM/b/dupe.bin"}}}
+        self.assertNotEqual(validate_pr.validate_file(rel, db, EMPTY_HASHES).outcome, "held")
 
     def test_the_file_the_database_holds_is_not_its_own_duplicate(self):
         """The documented example on a tracked file warned "Duplicate:
@@ -334,7 +352,8 @@ class Placement(_Fixture):
 class Reporting(_Fixture):
     def test_markdown_carries_every_digest_and_the_verdict(self):
         rel = self._bios_file("bios/Sega/Saturn/sega.bin")
-        result = validate_pr.validate_file(rel, None, EMPTY_HASHES)
+        probe = validate_pr.validate_file(rel, None, EMPTY_HASHES)
+        result = validate_pr.validate_file(rel, None, {**EMPTY_HASHES, "sha1": {probe.sha1}})
         markdown = result.to_markdown()
         for value in (result.sha1, result.md5, result.crc32):
             self.assertIn(value, markdown)
@@ -344,6 +363,26 @@ class Reporting(_Fixture):
         rel = self._bios_file("bios/Sega/Saturn/bad.exe")
         self.assertIn("### FAIL", validate_pr.validate_file(rel, None, EMPTY_HASHES).to_markdown())
 
+    @unittest.skipUnless((REPO_ROOT / "database.json").is_file(), "database.json is not built")
+    def test_the_status_line_names_what_is_left_to_review(self):
+        import shutil
+        import subprocess
+
+        held = REPO_ROOT / "bios" / "Sony" / "PlayStation Portable" / "ppge_atlas.zim"
+        if not held.is_file():
+            self.skipTest(f"{held} is not here")
+        shutil.copy(held, self.tmp / "ppge_atlas.zim")
+        (self.tmp / "mystery.bin").write_bytes(os.urandom(4096))
+        proc = subprocess.run(
+            [sys.executable, str(REPO_ROOT / "scripts" / "validate_pr.py"), "--markdown",
+             str(self.tmp / "mystery.bin"), str(self.tmp / "ppge_atlas.zim")],
+            capture_output=True, text=True, timeout=300, check=False,
+        )
+        self.assertEqual(proc.returncode, 0, proc.stderr)
+        self.assertIn("**Status**: no check failed, to review: 1 already held, "
+                      "1 not recognised", proc.stdout)
+        self.assertNotIn("all checks passed", proc.stdout)
+
 
 class PlatformHashLoading(_Fixture):
     def test_missing_directory_yields_empty_sets(self):