Refactor file handling to replace filename with name key and improve validation

Standardizes file specification by replacing the `filename` key with `name`. Enhances validation to handle malformed or incomplete file specifications gracefully. Adds error handling to catch unexpected exceptions and ensures temporary directories are cleaned up. Updates tests and documentation to reflect changes.

Author: m-messer

CLAUDE.md

@@ -104,8 +104,8 @@ All source lives in `evaluation_function/`:
 {
     "mode": "demo",
     "files": [
-        {"url": "https://.../data.csv?X-Amz-Signature=...", "filename": "data.csv"},
-        {"url": "https://.../helper.py?X-Amz-Signature=...", "filename": "helper.py"},
+        {"url": "https://.../data.csv?X-Amz-Signature=...", "name": "data.csv"},
+        {"url": "https://.../helper.py?X-Amz-Signature=...", "name": "helper.py"},
     ]
 }
 ```

evaluation_function/evaluation.py

@@ -3,7 +3,9 @@
 import os
 import shutil
 import subprocess
+import sys
 import tempfile
+import traceback
 from typing import Any
 
 import pycodestyle
@@ -328,13 +330,13 @@ def evaluation_function(response: Any, answer: Any, params: Params) -> Result:
         return result
 
     files_dir = None
-    file_warnings: list[str] = []
-    file_specs = params.get("files")
-    if file_specs:
-        files_dir = tempfile.mkdtemp()
-        file_warnings = download_files(file_specs, files_dir)
-
     try:
+        file_warnings: list[str] = []
+        file_specs = params.get("files")
+        if file_specs:
+            files_dir = tempfile.mkdtemp()
+            file_warnings = download_files(file_specs, files_dir)
+
         if mode == "demo":
             result = _evaluate_demo(str(response), result, files_dir)
         elif mode == "io_test":
@@ -343,21 +345,29 @@ def evaluation_function(response: Any, answer: Any, params: Params) -> Result:
         else:
             test_code = str(answer) if params.get("use_answer_as_test_code") else params.get("test_code", "")
             result = _evaluate_unit(str(response), test_code, result, files_dir=files_dir)
+
+        for warning in file_warnings:
+            result.add_feedback("error", warning)
+
+        pep8_param = params.get("pep8_feedback")
+        if pep8_param:
+            select = pep8_param if isinstance(pep8_param, list) else _PEP8_SELECT
+            violations = _check_pep8(str(response), select)
+            if violations:
+                body = "Style suggestions (PEP8):\n" + "\n".join(f"- {v}" for v in violations)
+            else:
+                body = "No style issues found."
+            result.add_feedback("style", body)
+    except Exception:
+        traceback.print_exc(file=sys.stderr)
+        result = Result()
+        result.add_feedback(
+            "error",
+            "An unexpected internal error occurred while evaluating this submission. "
+            "Please contact a course organizer.",
+        )
     finally:
         if files_dir is not None:
             shutil.rmtree(files_dir, ignore_errors=True)
 
-    for warning in file_warnings:
-        result.add_feedback("error", warning)
-
-    pep8_param = params.get("pep8_feedback")
-    if pep8_param:
-        select = pep8_param if isinstance(pep8_param, list) else _PEP8_SELECT
-        violations = _check_pep8(str(response), select)
-        if violations:
-            body = "Style suggestions (PEP8):\n" + "\n".join(f"- {v}" for v in violations)
-        else:
-            body = "No style issues found."
-        result.add_feedback("style", body)
-
     return result

evaluation_function/evaluation_test.py

@@ -1,4 +1,5 @@
 import os
+import tempfile
 import unittest
 from unittest.mock import patch
 
@@ -317,7 +318,7 @@ class TestFileDownloads(unittest.TestCase):
     @patch("evaluation_function.evaluation.download_files")
     def test_demo_mode_can_read_downloaded_file(self, mock_download):
         mock_download.side_effect = _stub_download({"data.csv": "1,2,3"})
-        params = {"mode": "demo", "files": [{"url": "https://example.com/k", "filename": "data.csv"}]}
+        params = {"mode": "demo", "files": [{"url": "https://example.com/k", "name": "data.csv"}]}
         result = evaluation_function("print(open('data.csv').read())", None, params).to_dict()
 
         self.assertIn("1,2,3", result["feedback"])
@@ -327,7 +328,7 @@ def test_io_test_downloads_once_for_all_tests(self, mock_download):
         mock_download.side_effect = _stub_download({"data.csv": "42"})
         params = {
             "mode": "io_test",
-            "files": [{"url": "https://example.com/k", "filename": "data.csv"}],
+            "files": [{"url": "https://example.com/k", "name": "data.csv"}],
             "tests": [_test("", "42\n"), _test("", "42\n")],
         }
         result = evaluation_function("print(open('data.csv').read())", None, params).to_dict()
@@ -341,7 +342,7 @@ def test_answer_code_receives_same_files(self, mock_download):
         params = {
             "mode": "io_test",
             "use_answer_as_expected_output": True,
-            "files": [{"url": "https://example.com/k", "filename": "data.csv"}],
+            "files": [{"url": "https://example.com/k", "name": "data.csv"}],
             "tests": [{"input": ""}],
         }
         code = "print(open('data.csv').read())"
@@ -352,15 +353,15 @@ def test_answer_code_receives_same_files(self, mock_download):
     @patch("evaluation_function.evaluation.download_files")
     def test_missing_file_reported_as_warning(self, mock_download):
         mock_download.return_value = ["File 'data.csv' could not be found."]
-        params = {"mode": "demo", "files": [{"url": "https://example.com/k", "filename": "data.csv"}]}
+        params = {"mode": "demo", "files": [{"url": "https://example.com/k", "name": "data.csv"}]}
         result = evaluation_function("print('hi')", None, params).to_dict()
 
         self.assertIn("could not be found", result["feedback"])
 
     @patch("evaluation_function.evaluation.download_files")
     def test_import_of_uploaded_module(self, mock_download):
         mock_download.side_effect = _stub_download({"helper.py": "def square(n):\n    return n * n\n"})
-        params = {"mode": "demo", "files": [{"url": "https://example.com/k", "filename": "helper.py"}]}
+        params = {"mode": "demo", "files": [{"url": "https://example.com/k", "name": "helper.py"}]}
         result = evaluation_function("import helper\nprint(helper.square(4))", None, params).to_dict()
 
         self.assertIn("16", result["feedback"])
@@ -371,36 +372,97 @@ def test_no_files_param_no_download_call(self):
             mock_download.assert_not_called()
 
 
+class TestMalformedFileSpec(unittest.TestCase):
+
+    def test_legacy_filename_key_does_not_crash(self):
+        # Reproduces the real-world crash report shape: a client sending the
+        # old/wrong "filename" key instead of "name". Must not crash.
+        params = {
+            "mode": "demo",
+            "files": [{
+                "url": "https://example.com/k",
+                "filename": "score_utils.py",
+                "type": "text/x-python-script",
+                "size": 237,
+            }],
+        }
+        result = evaluation_function("print('hi')", None, params).to_dict()
+
+        self.assertIn("hi", result["feedback"])
+        self.assertIn("missing", result["feedback"].lower())
+
+
+class TestUnexpectedExceptionHandling(unittest.TestCase):
+
+    @patch("evaluation_function.evaluation._run_code")
+    def test_unexpected_exception_during_evaluation_is_caught(self, mock_run):
+        mock_run.side_effect = RuntimeError("boom")
+
+        result = evaluation_function("print('hi')", None, {"mode": "demo"}).to_dict(include_test_data=True)
+
+        self.assertFalse(result["is_correct"])
+        self.assertIn("error", result["tags"])
+
+    @patch("evaluation_function.evaluation.download_files")
+    def test_exception_in_download_files_becomes_error_result(self, mock_download):
+        # Simulates a bug in download_files() itself (defense-in-depth,
+        # independent of the s3_files.py validation fix).
+        mock_download.side_effect = KeyError("name")
+        params = {"mode": "demo", "files": [{"url": "https://example.com/k", "name": "data.csv"}]}
+
+        result = evaluation_function("print('hi')", None, params).to_dict(include_test_data=True)
+
+        self.assertFalse(result["is_correct"])
+        self.assertIn("error", result["tags"])
+
+    def test_files_dir_cleaned_up_even_on_exception(self):
+        created_dirs = []
+        real_mkdtemp = tempfile.mkdtemp
+
+        def tracking_mkdtemp(*args, **kwargs):
+            d = real_mkdtemp(*args, **kwargs)
+            created_dirs.append(d)
+            return d
+
+        with patch("evaluation_function.evaluation.download_files", side_effect=RuntimeError("boom")), \
+                patch("evaluation_function.evaluation.tempfile.mkdtemp", side_effect=tracking_mkdtemp):
+            params = {"mode": "demo", "files": [{"url": "https://example.com/k", "name": "data.csv"}]}
+            evaluation_function("print('hi')", None, params)
+
+        self.assertTrue(created_dirs)
+        self.assertFalse(os.path.exists(created_dirs[0]))
+
+
 class TestFileAccessSandbox(unittest.TestCase):
 
     @patch("evaluation_function.evaluation.download_files")
     def test_read_downloaded_file_succeeds(self, mock_download):
         mock_download.side_effect = _stub_download({"data.csv": "hello"})
-        params = {"mode": "demo", "files": [{"url": "https://example.com/k", "filename": "data.csv"}]}
+        params = {"mode": "demo", "files": [{"url": "https://example.com/k", "name": "data.csv"}]}
         result = evaluation_function("print(open('data.csv').read())", None, params).to_dict()
 
         self.assertIn("hello", result["feedback"])
 
     @patch("evaluation_function.evaluation.download_files")
     def test_write_mode_to_provided_file_blocked(self, mock_download):
         mock_download.side_effect = _stub_download({"data.csv": "hello"})
-        params = {"mode": "demo", "files": [{"url": "https://example.com/k", "filename": "data.csv"}]}
+        params = {"mode": "demo", "files": [{"url": "https://example.com/k", "name": "data.csv"}]}
         result = evaluation_function("open('data.csv', 'w')", None, params).to_dict()
 
         self.assertIn("read-only", result["feedback"])
 
     @patch("evaluation_function.evaluation.download_files")
     def test_write_new_file_in_run_dir_blocked(self, mock_download):
         mock_download.side_effect = _stub_download({"data.csv": "hello"})
-        params = {"mode": "demo", "files": [{"url": "https://example.com/k", "filename": "data.csv"}]}
+        params = {"mode": "demo", "files": [{"url": "https://example.com/k", "name": "data.csv"}]}
         result = evaluation_function("open('output.txt', 'w')", None, params).to_dict()
 
         self.assertIn("read-only", result["feedback"])
 
     @patch("evaluation_function.evaluation.download_files")
     def test_pathlib_read_respects_sandbox(self, mock_download):
         mock_download.side_effect = _stub_download({"data.csv": "world"})
-        params = {"mode": "demo", "files": [{"url": "https://example.com/k", "filename": "data.csv"}]}
+        params = {"mode": "demo", "files": [{"url": "https://example.com/k", "name": "data.csv"}]}
         code = "from pathlib import Path\nprint(Path('data.csv').read_text())"
         result = evaluation_function(code, None, params).to_dict()
 
@@ -409,7 +471,7 @@ def test_pathlib_read_respects_sandbox(self, mock_download):
     @patch("evaluation_function.evaluation.download_files")
     def test_pathlib_write_respects_sandbox(self, mock_download):
         mock_download.side_effect = _stub_download({"data.csv": "world"})
-        params = {"mode": "demo", "files": [{"url": "https://example.com/k", "filename": "data.csv"}]}
+        params = {"mode": "demo", "files": [{"url": "https://example.com/k", "name": "data.csv"}]}
         code = "from pathlib import Path\nPath('data.csv').write_text('nope')"
         result = evaluation_function(code, None, params).to_dict()
 

evaluation_function/s3_files.py

@@ -12,7 +12,7 @@
 
 class FileSpec(TypedDict):
     url: str
-    filename: str
+    name: str
 
 
 def _valid_filename(filename: str) -> bool:
@@ -69,8 +69,25 @@ def download_files(files: list[FileSpec], dest_dir: str) -> list[str]:
     total_bytes = 0
 
     for spec in files:
-        url = spec["url"]
-        filename = spec["filename"]
+        if not isinstance(spec, dict):
+            warnings.append(
+                "One of the provided files is missing required information (url/name) and was skipped."
+            )
+            continue
+
+        url = spec.get("url")
+        filename = spec.get("name")
+        has_url = isinstance(url, str) and bool(url)
+        has_filename = isinstance(filename, str) and bool(filename)
+
+        if not has_url or not has_filename:
+            if has_filename:
+                warnings.append(f"File '{filename}' is missing a valid URL and was not made available.")
+            else:
+                warnings.append(
+                    "One of the provided files is missing required information (url/name) and was skipped."
+                )
+            continue
 
         if not _valid_filename(filename):
             warnings.append(f"File '{filename}' has an invalid filename and was not made available.")

evaluation_function/s3_files_test.py

@@ -45,7 +45,7 @@ def test_no_files_returns_empty(self):
     def test_successful_download_writes_file(self, mock_get):
         mock_get.return_value = _fake_response(b"hello", content_length=5)
 
-        warnings = download_files([{"url": _URL, "filename": "data.csv"}], self.dest_dir)
+        warnings = download_files([{"url": _URL, "name": "data.csv"}], self.dest_dir)
 
         self.assertEqual(warnings, [])
         with open(os.path.join(self.dest_dir, "data.csv"), "rb") as f:
@@ -55,7 +55,7 @@ def test_successful_download_writes_file(self, mock_get):
     def test_oversized_via_header_skipped(self, mock_get):
         mock_get.return_value = _fake_response(b"x" * 10, content_length=_MAX_FILE_BYTES + 1)
 
-        warnings = download_files([{"url": _URL, "filename": "big.csv"}], self.dest_dir)
+        warnings = download_files([{"url": _URL, "name": "big.csv"}], self.dest_dir)
 
         self.assertEqual(len(warnings), 1)
         self.assertIn("big.csv", warnings[0])
@@ -67,7 +67,7 @@ def test_oversized_via_streaming_skipped(self, mock_get):
         big_content = b"x" * (_MAX_FILE_BYTES + 1)
         mock_get.return_value = _fake_response(big_content, content_length=10)
 
-        warnings = download_files([{"url": _URL, "filename": "big.csv"}], self.dest_dir)
+        warnings = download_files([{"url": _URL, "name": "big.csv"}], self.dest_dir)
 
         self.assertEqual(len(warnings), 1)
         self.assertIn("big.csv", warnings[0])
@@ -79,7 +79,7 @@ def test_total_size_cap_skips_later_files(self, mock_get):
         # _MAX_TOTAL_BYTES (allowed), the 5th is skipped without a request.
         mock_get.return_value = _fake_response(b"x" * _MAX_FILE_BYTES, content_length=_MAX_FILE_BYTES)
 
-        files = [{"url": _URL, "filename": f"{i}.csv"} for i in range(5)]
+        files = [{"url": _URL, "name": f"{i}.csv"} for i in range(5)]
         warnings = download_files(files, self.dest_dir)
 
         self.assertEqual(mock_get.call_count, 4)
@@ -96,8 +96,8 @@ def side_effect(url, stream, timeout):
         mock_get.side_effect = side_effect
 
         files = [
-            {"url": "https://example.com/missing", "filename": "missing.csv"},
-            {"url": "https://example.com/ok", "filename": "ok.csv"},
+            {"url": "https://example.com/missing", "name": "missing.csv"},
+            {"url": "https://example.com/ok", "name": "ok.csv"},
         ]
         warnings = download_files(files, self.dest_dir)
 
@@ -109,20 +109,57 @@ def side_effect(url, stream, timeout):
     def test_network_error_skipped(self, mock_get):
         mock_get.side_effect = requests.exceptions.ConnectionError("boom")
 
-        warnings = download_files([{"url": _URL, "filename": "data.csv"}], self.dest_dir)
+        warnings = download_files([{"url": _URL, "name": "data.csv"}], self.dest_dir)
 
         self.assertEqual(len(warnings), 1)
         self.assertIn("data.csv", warnings[0])
 
     def test_rejects_non_https_url(self):
         for bad_url in ("http://example.com/data.csv", "file:///etc/passwd", "ftp://example.com/data.csv"):
-            warnings = download_files([{"url": bad_url, "filename": "data.csv"}], self.dest_dir)
+            warnings = download_files([{"url": bad_url, "name": "data.csv"}], self.dest_dir)
             self.assertEqual(len(warnings), 1, f"expected a warning for url={bad_url!r}")
 
     def test_filename_validation_rejects_traversal(self):
         for bad_name in ("../evil.py", "/etc/passwd", "", ".", ".."):
-            warnings = download_files([{"url": _URL, "filename": bad_name}], self.dest_dir)
-            self.assertEqual(len(warnings), 1, f"expected a warning for filename={bad_name!r}")
+            warnings = download_files([{"url": _URL, "name": bad_name}], self.dest_dir)
+            self.assertEqual(len(warnings), 1, f"expected a warning for name={bad_name!r}")
+
+    def test_legacy_filename_key_skipped_not_raised(self):
+        # Reproduces the real-world crash report: client actually sends "name",
+        # not the old "filename" key. A spec using the wrong/legacy key must
+        # not raise KeyError — it should be skipped with a warning.
+        warnings = download_files(
+            [{"url": _URL, "filename": "score_utils.py", "type": "text/x-python-script", "size": 237}],
+            self.dest_dir,
+        )
+        self.assertEqual(len(warnings), 1)
+        self.assertIn("missing", warnings[0].lower())
+
+    def test_missing_url_key_skipped_not_raised(self):
+        warnings = download_files([{"name": "data.csv"}], self.dest_dir)
+        self.assertEqual(len(warnings), 1)
+        self.assertIn("data.csv", warnings[0])
+
+    def test_non_dict_spec_skipped_not_raised(self):
+        warnings = download_files(["not-a-dict", 42, None], self.dest_dir)
+        self.assertEqual(len(warnings), 3)
+
+    def test_empty_spec_generic_message(self):
+        warnings = download_files([{}], self.dest_dir)
+        self.assertEqual(len(warnings), 1)
+        self.assertIn("missing", warnings[0].lower())
+
+    @patch("evaluation_function.s3_files.requests.get")
+    def test_malformed_spec_skipped_others_continue(self, mock_get):
+        mock_get.return_value = _fake_response(b"ok", content_length=2)
+        files = [
+            {"url": _URL, "filename": "score_utils.py"},  # wrong/legacy key, missing "name"
+            {"url": _URL, "name": "ok.csv"},
+        ]
+        warnings = download_files(files, self.dest_dir)
+
+        self.assertEqual(len(warnings), 1)
+        self.assertTrue(os.path.exists(os.path.join(self.dest_dir, "ok.csv")))
 
 
 if __name__ == "__main__":