Merge pull request #86 from VH-Lab/claude/did-python-matlab-sync-vlzx8g

Close the fail-open in check_files' step 3 (DID-matlab#200)

Author: stevevanhooser

src/did/did_matlab_python_bridge.yaml

@@ -40,7 +40,7 @@ classes:
   - name: database
     type: class
     matlab_path: "+did/database.m"
-    matlab_last_sync_hash: "ec1e733"
+    matlab_last_sync_hash: "9b36124"
     status: ported
     python_path: "did/database.py"
     python_class: "Database"
@@ -59,6 +59,14 @@ classes:
       MissingOptionalDependencyWarning gated on the
       DID_FORCE_VALIDATION_WARNINGS environment variable.
 
+      2026-09-09: bumped through DID-matlab 9b36124 (PR #200). Only
+      check_files needed a change -- the fail-open on a required file
+      declared in file_list with nothing bound to it in file_info, same
+      shape as PR #182's step-1 fail-open, one step down. See the checkfiles
+      method entry for the port. The rest of PR #200 was MATLAB-only: a
+      try/catch that read file_info before file_list under one arm, which
+      Python does not share.
+
       Reviewed 2026-08-29 through MATLAB c0a9507, nothing to port. Two
       MATLAB fixes landed in this window and Python has neither defect:
 
@@ -768,6 +776,20 @@ classes:
           meant a coverage check could not tell that isfilenamematch and
           canfindonefile were bridged at all. Split 2026-08-28.
 
+          2026-09-09 (DID-matlab#200 / #199): step 3 used to loop over the
+          actual file_info entries and fall through to is_valid = True when a
+          required name had none -- the same fail-open PR #182 closed for
+          step 1, one step further down. Ported here: check_files now
+          separates the two absences, keeping the file_list message for a
+          name missing from the list and adding a new one for a name that IS
+          in the file_list with nothing bound for it (naming the file and
+          file_info). The other half of DID-matlab#200, an independent read
+          of file_list before file_info, is not a bug on the Python side --
+          _validate_files already reads the two fields independently and did
+          not share MATLAB's try/catch shape -- so no code change beyond
+          check_files was needed. Coverage:
+          tests/test_validation.py:TestFileValidationDiagnosis.
+
       - name: isfilenamematch
         kind: static
         python_name: is_filename_match

src/did/validate.py

@@ -640,6 +640,15 @@ def check_files(
 
     Returns ``(is_valid, error_message)``. Mirrors MATLAB
     ``database.checkfiles``.
+
+    Steps 1 and 3 report DIFFERENT ABSENCES. A schema-required name can be
+    missing from the document's file_list (step 1), or it can be there in the
+    file_list -- declared, correctly -- with nothing bound to it in file_info
+    (step 3). The second is the common one, and step 3 used to fall out of its
+    empty match loop without raising: a required file was absent and the
+    document was accepted anyway. That is the same fail-open MATLAB PR #182
+    closed for step 1, one step further down, and DID-matlab#199 / #200 closed
+    for step 3.
     """
     expected_unique = sorted(set(expected_names))
     actual_unique = sorted(set(actual_file_names))
@@ -665,9 +674,23 @@ def check_files(
         if not _is_truthy(expected_value):
             continue
         item_name = expected_names[index]
-        for file_index, actual in enumerate(actual_file_names):
-            if not is_filename_match(item_name, actual):
-                continue
+        matches = [
+            file_index
+            for file_index, actual in enumerate(actual_file_names)
+            if is_filename_match(item_name, actual)
+        ]
+        if not matches:
+            # The name IS in the document's file_list -- step 1 above
+            # established that -- but nothing is bound to it in file_info, so
+            # there is no file to look for. Say that, and say it about
+            # file_info: the file_list is correct here, and blaming it is what
+            # cost the debugging time in DID-matlab#199.
+            return False, (
+                f'Required file "{item_name}" is declared in the file_list of '
+                f"{doc_name} but no file is bound to that name "
+                f"(files.file_info has no entry for it)"
+            )
+        for file_index in matches:
             entry = files[file_index] if file_index < len(files) else {}
             locations = entry.get("locations") if isinstance(entry, dict) else None
             if not can_find_one_file(locations):

tests/test_validation.py

@@ -404,3 +404,140 @@ def test_the_url_survives_the_round_trip(self):
             entries = entries if isinstance(entries, list) else [entries]
             locations.extend(item.get("location") for item in entries)
         self.assertIn(self.URL, locations)
+
+
+class TestFileValidationDiagnosis(_DatabaseTestCase):
+    """WHICH FIELD a file-validation failure blames.
+
+    check_files' step 3 used to loop over the actual file_info entries: a
+    required name declared in the file_list with nothing bound to it in
+    file_info matched nothing and fell through to is_valid = True. That is
+    the same fail-open MATLAB PR #182 closed for step 1, one step further
+    down; DID-matlab#199 closed it for step 3. Python's _validate_files
+    already read file_info and file_list independently -- MATLAB's own bug
+    from that PR was a try/catch shape Python does not share -- but the
+    fail-open in check_files itself was shared, so the port is that fix.
+    """
+
+    def _doc_with_file_info(self, file_info, file_list=None):
+        """A demoFile document whose files section is set by the caller.
+
+        demoFile declares filename1.ext and filename2.ext, both
+        mustbenotempty.
+        """
+        doc = Document("demoFile", **{"demoFile.value": 1})
+        doc.document_properties["files"]["file_info"] = file_info
+        if file_list is not None:
+            doc.document_properties["files"]["file_list"] = file_list
+        return doc
+
+    def _assert_refused(self, doc):
+        with self.assertRaises(ValidationError) as caught:
+            self.db.add_docs([doc])
+        self.assertEqual(
+            caught.exception.identifier,
+            "DID:Database:ValidationFiles",
+            f"refused for the wrong reason: {caught.exception}",
+        )
+        return str(caught.exception)
+
+    def test_empty_file_info_blames_the_unbound_file_not_the_file_list(self):
+        # The reported case. file_list declares both names and is correct;
+        # nothing is bound to them. file_info = [] used to fall out of the
+        # empty match loop and be accepted (Python) or throw and get the
+        # file_list blamed (MATLAB).
+        doc = self._doc_with_file_info([])
+
+        msg = self._assert_refused(doc)
+
+        self.assertIn(
+            "filename1.ext",
+            msg,
+            "the message must name the file that is not bound",
+        )
+        self.assertIn(
+            "file_info",
+            msg,
+            "the message must name the field that is actually empty",
+        )
+        self.assertNotIn(
+            "from the file_list",
+            msg,
+            (
+                "the file_list declares the file and is correct, so the "
+                f"message must not send the reader there: {msg}"
+            ),
+        )
+
+    def test_name_missing_from_file_list_names_the_absent_name(self):
+        # A document whose file_list is genuinely incomplete: it holds
+        # filename1.ext and not filename2.ext. The absent name is
+        # filename2.ext, and that is what the message must say.
+        doc = self._doc_with_file_info([], file_list=["filename1.ext"])
+
+        msg = self._assert_refused(doc)
+
+        self.assertIn(
+            "file_list",
+            msg,
+            "a name absent from the file_list IS a file_list problem",
+        )
+        self.assertIn(
+            "filename2.ext",
+            msg,
+            "the message must name the file that is absent from the list",
+        )
+        self.assertNotIn(
+            "filename1.ext",
+            msg,
+            (
+                "filename1.ext is in the file_list; only filename2.ext is "
+                f"absent from it: {msg}"
+            ),
+        )
+
+    def test_document_with_no_files_field_still_validates(self):
+        # Most classes declare no files at all, and their documents have no
+        # files section. That must keep working.
+        doc = Document("demoA", **{"demoA.value": 1})
+        self.assertNotIn(
+            "files",
+            doc.document_properties,
+            "precondition: a demoA document has no files section",
+        )
+        self.db.add_docs([doc])
+        self.assertIsNotNone(self.db.get_docs(doc.id()))
+
+    def test_declared_optional_files_need_not_be_bound(self):
+        # demoSeries declares both of its files mustbenotempty = 0, so a
+        # document that binds neither is valid: the file_list is correct
+        # and nothing it declares is required.
+        doc = Document("demoSeries", **{"demoSeries.value": 1})
+        doc.document_properties["files"]["file_info"] = []
+        self.db.add_docs([doc])
+        self.assertIsNotNone(self.db.get_docs(doc.id()))
+
+    def test_bound_required_files_validate(self):
+        # The other side of the tightening: a document that binds what its
+        # schema requires still validates. Validation does no file I/O, so
+        # the locations need not exist yet.
+        doc = Document("demoFile", **{"demoFile.value": 1})
+        doc.add_file("filename1.ext", "placeholder", ingest=0, delete_original=0)
+        doc.add_file("filename2.ext", "placeholder", ingest=0, delete_original=0)
+        self.db.add_docs([doc])
+        self.assertIsNotNone(self.db.get_docs(doc.id()))
+
+    def test_check_files_directly_reports_unbound_required_file(self):
+        # The unit-level counterpart, calling check_files without a database.
+        is_valid, message = check_files(
+            expected_names=["filename1.ext", "filename2.ext"],
+            must_have_value=[1, 1],
+            actual_file_names=[],
+            doc_name="demoFile doc X",
+            files=[],
+            actual_file_list=["filename1.ext", "filename2.ext"],
+        )
+        self.assertFalse(is_valid)
+        self.assertIn("filename1.ext", message)
+        self.assertIn("file_info", message)
+        self.assertNotIn("from the file_list", message)