Add S3 file download support to evaluation pipeline

Implements `params["files"]` feature to allow downloading S3 objects into a per-request working directory. Updates `evaluation.py` and security logic to enable read-only file access. Introduces `s3_files.py` for managing S3 interactions and accompanying unit tests in `s3_files_test.py`. Expands documentation in `CLAUDE.md` and adds integration tests to verify functionality.

Author: m-messer

CLAUDE.md

@@ -11,17 +11,19 @@ All source lives in `evaluation_function/`:
 | `main.py` | IPC server entry point; registers `evaluation_function` and `preview_function` with lf_toolkit |
 | `evaluation.py` | Core evaluation pipeline: security check → subprocess execution → output comparison → S3 plot upload → structured feedback |
 | `preview.py` | AST-based pre-execution security validator (`_SecurityVisitor`) |
+| `s3_files.py` | Downloads `params["files"]` objects from S3 into the per-request working directory |
 | `dev.py` | CLI wrapper for local manual testing |
 
 ### Evaluation pipeline (`evaluation.py`)
 
 1. Run AST security check on student code
-2. Dispatch by `params["mode"]` (required):
+2. If `params["files"]` is set, download the listed S3 objects once into a per-request working directory (see `s3_files.py`), used as the subprocess `cwd` for every run in this request
+3. Dispatch by `params["mode"]` (required):
    - **`demo`**: execute code with no stdin; return stdout/plots as `output` feedback (no pass/fail)
    - **`io_test`**: for each test in `params["tests"]`, execute with `test["input"]` as stdin and compare stdout against `test["expected_output"]`; upload matplotlib plots on pass or fail
    - **`unit_test`**: append `params["test_code"]` + unit-runner harness to student code; execute once; parse JSON results; supports plain `test_*` functions, `unittest.TestCase` subclasses, and Hypothesis-based tests
-3. Upload any captured matplotlib figures to S3 (`_UPLOAD_FOLDER = "evaluatePython"`)
-4. Return a `Result` with feedback tags: `pass`, `fail`, `hidden_fail`, `error`, `output`, `summary`
+4. Upload any captured matplotlib figures to S3 (`_UPLOAD_FOLDER = "evaluatePython"`)
+5. Return a `Result` with feedback tags: `pass`, `fail`, `hidden_fail`, `error`, `output`, `summary`
 
 ### Request shape
 
@@ -90,16 +92,33 @@ All source lives in `evaluation_function/`:
     "pep8_feedback": ["E225", "E231"],  # custom rule list
     "tests": [...]
 }
+
+# files — optional, works with all modes
+# Downloads objects from S3 into a per-request working directory (the
+# subprocess's cwd) before student code runs. Data files can be read with
+# open()/pandas.read_csv()/etc.; .py files are importable by student code
+# since they're co-located with the generated script. The same files are
+# also available to the answer code when use_answer_as_expected_output /
+# use_answer_as_test_code is set. Requires the S3_FILES_BUCKET env var.
+{
+    "mode": "demo",
+    "files": [
+        {"key": "uploads/<question-id>/data.csv", "filename": "data.csv"},
+        {"key": "uploads/<question-id>/helper.py", "filename": "helper.py"},
+    ]
+}
 ```
 
 ### Security model (`preview.py`)
 
-`_SecurityVisitor` walks the AST before any execution and blocks:
+`_SecurityVisitor` walks the AST and blocks:
 
-- **Modules**: `os`, `sys`, `subprocess`, `socket`, `urllib`, `http`, `requests`, `shutil`, `pathlib`, `ftplib`, `smtplib`, `ctypes`, `multiprocessing`, `threading`, `importlib`, `pickle`, `builtins`
-- **Builtins**: `exec`, `eval`, `compile`, `open`, `__import__`
+- **Modules**: `os`, `sys`, `subprocess`, `socket`, `urllib`, `http`, `requests`, `shutil`, `ftplib`, `smtplib`, `ctypes`, `multiprocessing`, `threading`, `importlib`, `pickle`, `builtins`
+- **Builtins**: `exec`, `eval`, `compile`, `__import__`
 - **Dunder attribute access**: any `__attr__` style attribute
 
+`open`/`pathlib` are intentionally **not** blocked here — they're needed to read files loaded via `params["files"]` (see above). **Important caveat**: `preview_function` (this check) and `evaluation_function` (actual grading) are registered as two independent RPC methods in `main.py`; `evaluation.py` never calls `preview.py`. This check only powers editor-time linting feedback — it does not gate what code can do at grading time. The real, load-bearing control for file access is a runtime-injected restricted `open`/`io.open` in `evaluation.py`'s subprocess preamble (`_safe_open`), which blocks *write* access to anything inside the per-run files directory. It is not a hard sandbox boundary — since `os`/`subprocess` remain fully importable and runnable at grading time regardless of this feature, a student can bypass file restrictions entirely via `os`. Treat this as scoping the intended file-access path, not as isolation.
+
 ## Key commands
 
 ```bash
@@ -148,7 +167,8 @@ CI runs on Python 3.12 and uploads JUnit XML results (`.github/workflows/test-li
 | `FUNCTION_ARGS` | `-m,evaluation_function.main` | lf_toolkit runner |
 | `FUNCTION_RPC_TRANSPORT` | `ipc` | lf_toolkit transport |
 | `LOG_LEVEL` | `debug` | Logging verbosity |
-| `AWS_*` / boto3 credentials | Runtime env | Required for S3 plot uploads |
+| `AWS_*` / boto3 credentials | Runtime env | Required for S3 plot uploads and `files` downloads |
+| `S3_FILES_BUCKET` | Runtime env | Bucket name (not URI) that `params["files"]` object keys are resolved against |
 
 Dependencies managed via Poetry; `.venv` is created in-project (`poetry.toml`).
 

evaluation_function/evaluation.py

@@ -11,6 +11,8 @@
 from lf_toolkit.evaluation import Result, Params
 from lf_toolkit.evaluation.image_upload import upload_image, ImageUploadError
 
+from .s3_files import download_files, FileDownloadError
+
 _TIMEOUT = 25
 _UPLOAD_FOLDER = "evaluatePython"
 
@@ -30,10 +32,27 @@ def error(self, line_number, offset, text, check):
 
 _PREAMBLE_TEMPLATE = """\
 import os as _os
+import io as _io
+import builtins as _builtins
 
 _plot_dir = {plot_dir!r}
 _plot_idx = [0]
 
+_files_dir = _os.path.realpath({files_dir!r})
+_real_open = _builtins.open
+
+def _safe_open(file, mode="r", *args, **kwargs):
+    if isinstance(file, (str, _os.PathLike)) and any(m in mode for m in ("w", "a", "x", "+")):
+        _target = _os.path.realpath(_os.path.join(_files_dir, _os.fspath(file)))
+        if _os.path.commonpath([_target, _files_dir]) == _files_dir:
+            raise PermissionError("Provided files are read-only and cannot be modified.")
+    return _real_open(file, mode, *args, **kwargs)
+
+# pathlib.Path.open()/read_text()/write_text() call io.open(...) directly,
+# not the builtins.open name, so both bindings must be patched.
+_builtins.open = _safe_open
+_io.open = _safe_open
+
 def _capture_plots():
     import sys as _sys
     if 'matplotlib.pyplot' not in _sys.modules:
@@ -107,19 +126,22 @@ def _add_repl_print(code: str) -> str:
     return code + f"\nprint(repr({ast.unparse(node)}))"
 
 
-def _run_code(code: str, stdin: str) -> tuple[str, str, bool, list[Image.Image]]:
+def _run_code(code: str, stdin: str, files_dir: str | None = None) -> tuple[str, str, bool, list[Image.Image]]:
     plot_dir = tempfile.mkdtemp()
-    preamble = _PREAMBLE_TEMPLATE.format(plot_dir=plot_dir)
-    with tempfile.NamedTemporaryFile(mode="w", suffix=".py", delete=False) as f:
+    own_run_dir = files_dir is None
+    run_dir = files_dir if files_dir is not None else tempfile.mkdtemp()
+    preamble = _PREAMBLE_TEMPLATE.format(plot_dir=plot_dir, files_dir=run_dir)
+    script_path = os.path.join(run_dir, "_submission.py")
+    with open(script_path, "w") as f:
         f.write(preamble + "\n" + code + "\n" + _CAPTURE_CALL)
-        tmpfile = f.name
     try:
         proc = subprocess.run(
-            ["python", tmpfile],
+            ["python", "_submission.py"],
             input=stdin,
             capture_output=True,
             text=True,
             timeout=_TIMEOUT,
+            cwd=run_dir,
             env={**os.environ, "MPLBACKEND": "Agg", "MPLCONFIGDIR": "/tmp"},
         )
         images = []
@@ -133,8 +155,10 @@ def _run_code(code: str, stdin: str) -> tuple[str, str, bool, list[Image.Image]]
     except subprocess.TimeoutExpired:
         return "", "", True, []
     finally:
-        os.unlink(tmpfile)
+        os.unlink(script_path)
         shutil.rmtree(plot_dir, ignore_errors=True)
+        if own_run_dir:
+            shutil.rmtree(run_dir, ignore_errors=True)
 
 
 def _code_block(label: str, content: str) -> str:
@@ -167,9 +191,9 @@ def _check_pep8(code: str, select: list[str]) -> list[str]:
     return [f"Line {ln}: {text}" for ln, text in checker.report.violations]
 
 
-def _evaluate_demo(response: str, result: Result) -> Result:
+def _evaluate_demo(response: str, result: Result, files_dir: str | None = None) -> Result:
     response = _add_repl_print(response)
-    stdout, stderr, timed_out, images = _run_code(response, "")
+    stdout, stderr, timed_out, images = _run_code(response, "", files_dir)
     if timed_out:
         result.add_feedback("error", f"Code timed out after {_TIMEOUT}s.")
     elif stderr and not stdout:
@@ -181,7 +205,7 @@ def _evaluate_demo(response: str, result: Result) -> Result:
     return result
 
 
-def _evaluate_io(response: str, tests: list, result: Result, answer: str = "") -> Result:
+def _evaluate_io(response: str, tests: list, result: Result, answer: str = "", files_dir: str | None = None) -> Result:
     passed = 0
     response = _add_repl_print(response)
 
@@ -204,12 +228,12 @@ def _evaluate_io(response: str, tests: list, result: Result, answer: str = "") -
         if answer:
             ans_code = _add_repl_print(answer)
             ans_run_code = (prefix + ans_code) if inject else ans_code
-            ans_stdout, _, _, _ = _run_code(ans_run_code, run_stdin)
+            ans_stdout, _, _, _ = _run_code(ans_run_code, run_stdin, files_dir)
             expected = ans_stdout.rstrip()
         else:
             expected = test.get("expected_output", "").rstrip()
 
-        stdout, stderr, timed_out, images = _run_code(run_code, run_stdin)
+        stdout, stderr, timed_out, images = _run_code(run_code, run_stdin, files_dir)
         actual = stdout.rstrip()
         label = f"Hidden test {i}" if hidden else f"Test {i}"
 
@@ -246,15 +270,15 @@ def _evaluate_io(response: str, tests: list, result: Result, answer: str = "") -
     return result
 
 
-def _evaluate_unit(response: str, test_code: str, result: Result) -> Result:
+def _evaluate_unit(response: str, test_code: str, result: Result, files_dir: str | None = None) -> Result:
     if not test_code.strip():
         result.add_feedback("error", "No test code provided for unit_test mode.")
         return result
 
     results_path = tempfile.mktemp(suffix=".json")
     runner = _UNIT_RUNNER_TEMPLATE.format(results_path=results_path)
     combined = _add_repl_print(response) + "\n\n" + test_code + runner
-    stdout, stderr, timed_out, _ = _run_code(combined, "")
+    stdout, stderr, timed_out, _ = _run_code(combined, "", files_dir)
 
     test_results = None
     try:
@@ -303,14 +327,31 @@ def evaluation_function(response: Any, answer: Any, params: Params) -> Result:
         result.add_feedback("error", f"Unknown or missing mode: {mode!r}. Expected 'demo', 'io_test', or 'unit_test'.")
         return result
 
-    if mode == "demo":
-        result = _evaluate_demo(str(response), result)
-    elif mode == "io_test":
-        ans = str(answer) if params.get("use_answer_as_expected_output") else ""
-        result = _evaluate_io(str(response), params.get("tests", []), result, answer=ans)
-    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 = None
+    file_warnings: list[str] = []
+    file_specs = params.get("files")
+    if file_specs:
+        files_dir = tempfile.mkdtemp()
+        try:
+            file_warnings = download_files(file_specs, files_dir)
+        except FileDownloadError as e:
+            file_warnings = [str(e)]
+
+    try:
+        if mode == "demo":
+            result = _evaluate_demo(str(response), result, files_dir)
+        elif mode == "io_test":
+            ans = str(answer) if params.get("use_answer_as_expected_output") else ""
+            result = _evaluate_io(str(response), params.get("tests", []), result, answer=ans, files_dir=files_dir)
+        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)
+    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:

evaluation_function/evaluation_test.py

@@ -1,3 +1,4 @@
+import os
 import unittest
 from unittest.mock import patch
 
@@ -302,6 +303,119 @@ def test_hypothesis_fail_shows_minimal_example(self):
         self.assertIn("square(", result["feedback"])
 
 
+def _stub_download(content_by_filename):
+    def fake_download(files, dest_dir):
+        for filename, content in content_by_filename.items():
+            with open(os.path.join(dest_dir, filename), "w") as f:
+                f.write(content)
+        return []
+    return fake_download
+
+
+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": [{"key": "k", "filename": "data.csv"}]}
+        result = evaluation_function("print(open('data.csv').read())", None, params).to_dict()
+
+        self.assertIn("1,2,3", result["feedback"])
+
+    @patch("evaluation_function.evaluation.download_files")
+    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": [{"key": "k", "filename": "data.csv"}],
+            "tests": [_test("", "42\n"), _test("", "42\n")],
+        }
+        result = evaluation_function("print(open('data.csv').read())", None, params).to_dict()
+
+        self.assertTrue(result["is_correct"])
+        mock_download.assert_called_once()
+
+    @patch("evaluation_function.evaluation.download_files")
+    def test_answer_code_receives_same_files(self, mock_download):
+        mock_download.side_effect = _stub_download({"data.csv": "7"})
+        params = {
+            "mode": "io_test",
+            "use_answer_as_expected_output": True,
+            "files": [{"key": "k", "filename": "data.csv"}],
+            "tests": [{"input": ""}],
+        }
+        code = "print(open('data.csv').read())"
+        result = evaluation_function(code, code, params).to_dict()
+
+        self.assertTrue(result["is_correct"])
+
+    @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": [{"key": "k", "filename": "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": [{"key": "k", "filename": "helper.py"}]}
+        result = evaluation_function("import helper\nprint(helper.square(4))", None, params).to_dict()
+
+        self.assertIn("16", result["feedback"])
+
+    def test_no_files_param_no_download_call(self):
+        with patch("evaluation_function.evaluation.download_files") as mock_download:
+            evaluation_function("print('hi')", None, {"mode": "demo"})
+            mock_download.assert_not_called()
+
+
+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": [{"key": "k", "filename": "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": [{"key": "k", "filename": "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": [{"key": "k", "filename": "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": [{"key": "k", "filename": "data.csv"}]}
+        code = "from pathlib import Path\nprint(Path('data.csv').read_text())"
+        result = evaluation_function(code, None, params).to_dict()
+
+        self.assertIn("world", result["feedback"])
+
+    @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": [{"key": "k", "filename": "data.csv"}]}
+        code = "from pathlib import Path\nPath('data.csv').write_text('nope')"
+        result = evaluation_function(code, None, params).to_dict()
+
+        self.assertIn("read-only", result["feedback"])
+
+
 class TestPep8Feedback(unittest.TestCase):
 
     def test_violations_reported(self):

evaluation_function/preview.py

@@ -4,12 +4,12 @@
 
 _BLOCKED_MODULES = {
     "os", "sys", "subprocess", "socket", "urllib", "http",
-    "requests", "shutil", "pathlib", "ftplib", "smtplib",
+    "requests", "shutil", "ftplib", "smtplib",
     "ctypes", "multiprocessing", "threading", "importlib",
     "pickle", "builtins",
 }
 
-_BLOCKED_BUILTINS = {"exec", "eval", "compile", "open", "__import__"}
+_BLOCKED_BUILTINS = {"exec", "eval", "compile", "__import__"}
 
 
 class _SecurityVisitor(ast.NodeVisitor):