Make Instructions parsing fail on invalid input instead of logging (#219)

Author: dangusev

agents-core/vision_agents/core/instructions.py

@@ -3,13 +3,17 @@
 import re
 from pathlib import Path
 
-__all__ = ["Instructions"]
+__all__ = ["Instructions", "InstructionsReadError"]
+
 
 logger = logging.getLogger(__name__)
 
 _INITIAL_CWD = os.getcwd()
 
-_MD_PATTERN = re.compile(r"@([^\s@]+\.md)")
+_MD_PATTERN = re.compile(r"@([^\s@]+)")
+
+
+class InstructionsReadError(Exception): ...
 
 
 class Instructions:
@@ -62,9 +66,7 @@ def _extract_full_reference(self) -> str:
             for filename, content in markdown_contents.items():
                 markdown_lines.append(f"\n### {filename}")
                 # Only include non-empty content
-                markdown_lines.append(
-                    content or "*(File not found or could not be read)*"
-                )
+                markdown_lines.append(content or "*(File is empty)*")
         full_reference = "\n".join(markdown_lines)
         return full_reference
 
@@ -86,30 +88,28 @@ def _read_md_file(self, file_path: str | Path) -> str:
 
         # Check if the path is a file, it exists, and it's a markdown file.
         skip_reason = ""
-        if not full_path.is_file():
+        if not full_path.exists():
+            skip_reason = "file not found"
+        elif not full_path.is_file():
             skip_reason = "path is not a file"
         elif full_path.name.startswith("."):
-            skip_reason = "path is invalid"
-        elif not full_path.exists():
-            skip_reason = "file not found"
+            skip_reason = 'filename cannot start with "."'
         elif full_path.suffix != ".md":
             skip_reason = "file is not .md"
         # The markdown file also must be inside the base_dir
         elif not full_path.is_relative_to(self._base_dir):
-            skip_reason = "file outside the base directory"
+            skip_reason = f"path outside the base directory {self._base_dir}"
 
         if skip_reason:
-            logger.debug(
-                f"Skip reading instructions from {full_path}. Reason: {skip_reason}"
+            raise InstructionsReadError(
+                f"Failed to read instructions from {full_path}; reason - {skip_reason}"
             )
-            return ""
 
         try:
             logger.info(f"Reading instructions from file {full_path}")
             with open(full_path, mode="r") as f:
                 return f.read()
-        except (OSError, IOError, UnicodeDecodeError):
-            logger.warning(
-                f"Failed to read instructions from file {full_path}", exc_info=True
-            )
-            return ""
+        except (OSError, IOError, UnicodeDecodeError) as exc:
+            raise InstructionsReadError(
+                f"Failed to read instructions from file {full_path}; reason - {exc}"
+            ) from exc

tests/test_instructions.py

@@ -1,5 +1,5 @@
 import pytest
-from vision_agents.core.instructions import Instructions
+from vision_agents.core.instructions import Instructions, InstructionsReadError
 
 
 class TestInstructions:
@@ -28,40 +28,29 @@ def test_parse_not_a_file(self, tmp_path):
         file_path = tmp_path / "file1.md"
         file_path.mkdir()
         input_text = "read @file1.md"
-        instructions = Instructions(input_text=input_text, base_dir=tmp_path)
-        assert instructions.input_text == input_text
-        assert (
-            instructions.full_reference
-            == f"{input_text}\n\n\n## Referenced Documentation:\n\n### file1.md\n*(File not found or could not be read)*"
-        )
+        with pytest.raises(InstructionsReadError, match="reason - path is not a file"):
+            Instructions(input_text=input_text, base_dir=tmp_path)
 
     def test_parse_file_doesnt_exist(self, tmp_path):
         input_text = "read @file1.md"
-        instructions = Instructions(input_text=input_text, base_dir=tmp_path)
-        assert instructions.input_text == input_text
-        assert (
-            instructions.full_reference
-            == f"{input_text}\n\n\n## Referenced Documentation:\n\n### file1.md\n*(File not found or could not be read)*"
-        )
+        with pytest.raises(InstructionsReadError, match="reason - file not found"):
+            Instructions(input_text=input_text, base_dir=tmp_path)
 
     def test_parse_file_not_md(self, tmp_path):
         input_text = "read @file1.txt"
         file_path = tmp_path / "file1.txt"
         file_path.write_text("abcdef", encoding="utf-8")
-        instructions = Instructions(input_text=input_text, base_dir=tmp_path)
-        assert instructions.input_text == input_text
-        assert instructions.full_reference == input_text
+        with pytest.raises(InstructionsReadError, match="reason - file is not .md"):
+            Instructions(input_text=input_text, base_dir=tmp_path)
 
     def test_parse_file_hidden(self, tmp_path):
         input_text = "read @.file1.md"
         file_path = tmp_path / ".file1.md"
         file_path.write_text("abcdef", encoding="utf-8")
-        instructions = Instructions(input_text=input_text, base_dir=tmp_path)
-        assert instructions.input_text == input_text
-        assert (
-            instructions.full_reference
-            == f"{input_text}\n\n\n## Referenced Documentation:\n\n### .file1.md\n*(File not found or could not be read)*"
-        )
+        with pytest.raises(
+            InstructionsReadError, match='reason - filename cannot start with "."'
+        ):
+            Instructions(input_text=input_text, base_dir=tmp_path)
 
     def test_parse_file_outside_base_dir(self, tmp_path):
         file_path1 = tmp_path / "file1.md"
@@ -73,9 +62,7 @@ def test_parse_file_outside_base_dir(self, tmp_path):
         file_path1.write_text("abcdef", encoding="utf-8")
         file_path2.write_text("abcdef", encoding="utf-8")
 
-        instructions = Instructions(input_text=input_text, base_dir=base_dir)
-        assert instructions.input_text == input_text
-        assert (
-            instructions.full_reference
-            == f"{input_text}\n\n\n## Referenced Documentation:\n\n### {file_path1}\n*(File not found or could not be read)*"
-        )
+        with pytest.raises(
+            InstructionsReadError, match="reason - path outside the base directory"
+        ):
+            Instructions(input_text=input_text, base_dir=base_dir)