From b9f1c991490ff827fb5e1aa738bb32ac6d7799be Mon Sep 17 00:00:00 2001 From: Marcus Messer Date: Thu, 13 Aug 2026 10:41:05 +0100 Subject: [PATCH 1/7] 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. --- CLAUDE.md | 34 +++++-- evaluation_function/evaluation.py | 83 ++++++++++++---- evaluation_function/evaluation_test.py | 114 +++++++++++++++++++++ evaluation_function/preview.py | 4 +- evaluation_function/preview_test.py | 23 ++++- evaluation_function/s3_files.py | 106 ++++++++++++++++++++ evaluation_function/s3_files_test.py | 131 +++++++++++++++++++++++++ 7 files changed, 464 insertions(+), 31 deletions(-) create mode 100644 evaluation_function/s3_files.py create mode 100644 evaluation_function/s3_files_test.py diff --git a/CLAUDE.md b/CLAUDE.md index b725a02..2d2222b 100644 --- a/CLAUDE.md +++ b/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//data.csv", "filename": "data.csv"}, + {"key": "uploads//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`). diff --git a/evaluation_function/evaluation.py b/evaluation_function/evaluation.py index 0811721..398ae64 100755 --- a/evaluation_function/evaluation.py +++ b/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,7 +270,7 @@ 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 @@ -254,7 +278,7 @@ def _evaluate_unit(response: str, test_code: str, result: Result) -> 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: diff --git a/evaluation_function/evaluation_test.py b/evaluation_function/evaluation_test.py index 4c9f492..fc7ee47 100755 --- a/evaluation_function/evaluation_test.py +++ b/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): diff --git a/evaluation_function/preview.py b/evaluation_function/preview.py index 190f362..32945b9 100755 --- a/evaluation_function/preview.py +++ b/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): diff --git a/evaluation_function/preview_test.py b/evaluation_function/preview_test.py index a8d7509..94bb20e 100755 --- a/evaluation_function/preview_test.py +++ b/evaluation_function/preview_test.py @@ -53,4 +53,25 @@ def test_input_is_allowed(self): result = preview_function(response, params) self.assertIn("preview", result) - self.assertNotIn("Unsafe", result["preview"].get("feedback", "")) \ No newline at end of file + self.assertNotIn("Unsafe", result["preview"].get("feedback", "")) + + def test_open_is_allowed(self): + response, params = "f = open('data.csv')\nf.read()", Params() + result = preview_function(response, params) + + self.assertIn("preview", result) + self.assertNotIn("Unsafe", result["preview"].get("feedback", "")) + + def test_pathlib_import_allowed(self): + response, params = "from pathlib import Path\nPath('data.csv').read_text()", Params() + result = preview_function(response, params) + + self.assertIn("preview", result) + self.assertNotIn("Unsafe", result["preview"].get("feedback", "")) + + def test_os_still_blocked(self): + response, params = "import os\nos.system('ls')", Params() + result = preview_function(response, params) + + self.assertIn("preview", result) + self.assertIn("Unsafe", result["preview"].get("feedback", "")) \ No newline at end of file diff --git a/evaluation_function/s3_files.py b/evaluation_function/s3_files.py new file mode 100644 index 0000000..0c61065 --- /dev/null +++ b/evaluation_function/s3_files.py @@ -0,0 +1,106 @@ +import os +from typing import TypedDict + +import boto3 +from botocore.config import Config +from botocore.exceptions import ClientError + +_MAX_FILE_BYTES = 5 * 1024 * 1024 +_MAX_TOTAL_BYTES = 20 * 1024 * 1024 +_DOWNLOAD_TIMEOUT = 10 + + +class FileSpec(TypedDict): + key: str + filename: str + + +class FileDownloadError(Exception): + """Raised for whole-request configuration problems (e.g. missing bucket env var).""" + pass + + +def _s3_client(): + return boto3.client( + "s3", + region_name=os.environ.get("AWS_REGION", "eu-west-2"), + config=Config( + connect_timeout=_DOWNLOAD_TIMEOUT, + read_timeout=_DOWNLOAD_TIMEOUT, + retries={"max_attempts": 2}, + ), + ) + + +def _get_bucket_name() -> str: + bucket = os.environ.get("S3_FILES_BUCKET") + if not bucket: + raise FileDownloadError("S3_FILES_BUCKET environment variable is not set") + return bucket + + +def _valid_filename(filename: str) -> bool: + if not filename or filename in (".", ".."): + return False + return os.path.basename(filename) == filename + + +def download_files(files: list[FileSpec], dest_dir: str) -> list[str]: + """Download each file into dest_dir. + + Returns a list of warning strings for files that were skipped (missing, + too large, or errored) — never raises for per-file problems, only for + whole-config problems (missing bucket env var). + """ + if not files: + return [] + + bucket = _get_bucket_name() + client = _s3_client() + + warnings: list[str] = [] + total_bytes = 0 + + for spec in files: + key = spec["key"] + filename = spec["filename"] + + if not _valid_filename(filename): + warnings.append(f"File '{filename}' has an invalid filename and was not made available.") + continue + + real_dest_dir = os.path.realpath(dest_dir) + target = os.path.realpath(os.path.join(real_dest_dir, filename)) + if os.path.commonpath([target, real_dest_dir]) != real_dest_dir: + warnings.append(f"File '{filename}' has an invalid filename and was not made available.") + continue + + try: + head = client.head_object(Bucket=bucket, Key=key) + except ClientError as e: + warnings.append(f"File '{filename}' could not be found or accessed ({e}).") + continue + + size = head.get("ContentLength", 0) + if size > _MAX_FILE_BYTES: + warnings.append( + f"File '{filename}' exceeds the {_MAX_FILE_BYTES // (1024 * 1024)}MB size limit " + "and was not made available." + ) + continue + if total_bytes + size > _MAX_TOTAL_BYTES: + warnings.append( + f"File '{filename}' was skipped because it would exceed the total " + f"{_MAX_TOTAL_BYTES // (1024 * 1024)}MB size limit for this run." + ) + continue + + try: + client.download_file(bucket, key, target) + except ClientError as e: + warnings.append(f"File '{filename}' could not be downloaded ({e}).") + continue + + total_bytes += size + + return warnings diff --git a/evaluation_function/s3_files_test.py b/evaluation_function/s3_files_test.py new file mode 100644 index 0000000..addd0e1 --- /dev/null +++ b/evaluation_function/s3_files_test.py @@ -0,0 +1,131 @@ +import os +import tempfile +import unittest +from unittest.mock import MagicMock, patch + +from botocore.exceptions import ClientError + +from .s3_files import download_files, FileDownloadError, _MAX_FILE_BYTES + + +def _client_error(): + return ClientError({"Error": {"Code": "404", "Message": "Not Found"}}, "HeadObject") + + +class TestDownloadFiles(unittest.TestCase): + + def setUp(self): + self.dest_dir = tempfile.mkdtemp() + self.env_patcher = patch.dict(os.environ, {"S3_FILES_BUCKET": "test-bucket"}) + self.env_patcher.start() + + def tearDown(self): + self.env_patcher.stop() + + def test_no_files_returns_empty_without_client(self): + warnings = download_files([], self.dest_dir) + self.assertEqual(warnings, []) + + @patch("evaluation_function.s3_files.boto3.client") + def test_successful_download_writes_file(self, mock_client_factory): + mock_client = MagicMock() + mock_client.head_object.return_value = {"ContentLength": 10} + + def fake_download(bucket, key, target): + with open(target, "w") as f: + f.write("hello") + + mock_client.download_file.side_effect = fake_download + mock_client_factory.return_value = mock_client + + warnings = download_files([{"key": "data.csv", "filename": "data.csv"}], self.dest_dir) + + self.assertEqual(warnings, []) + with open(os.path.join(self.dest_dir, "data.csv")) as f: + self.assertEqual(f.read(), "hello") + + @patch("evaluation_function.s3_files.boto3.client") + def test_oversized_file_skipped(self, mock_client_factory): + mock_client = MagicMock() + mock_client.head_object.return_value = {"ContentLength": _MAX_FILE_BYTES + 1} + mock_client_factory.return_value = mock_client + + warnings = download_files([{"key": "big.csv", "filename": "big.csv"}], self.dest_dir) + + self.assertEqual(len(warnings), 1) + self.assertIn("big.csv", warnings[0]) + mock_client.download_file.assert_not_called() + self.assertFalse(os.path.exists(os.path.join(self.dest_dir, "big.csv"))) + + @patch("evaluation_function.s3_files.boto3.client") + def test_total_size_cap_skips_later_files(self, mock_client_factory): + # 5 files at exactly the per-file cap: the first 4 sum to exactly + # _MAX_TOTAL_BYTES (allowed), the 5th would push over it (skipped). + mock_client = MagicMock() + mock_client.head_object.return_value = {"ContentLength": _MAX_FILE_BYTES} + mock_client_factory.return_value = mock_client + + files = [{"key": f"{i}.csv", "filename": f"{i}.csv"} for i in range(5)] + warnings = download_files(files, self.dest_dir) + + self.assertEqual(mock_client.download_file.call_count, 4) + self.assertEqual(len(warnings), 1) + self.assertIn("4.csv", warnings[0]) + + def test_missing_bucket_env_var_raises_only_when_files_present(self): + self.env_patcher.stop() + try: + with self.assertRaises(FileDownloadError): + download_files([{"key": "data.csv", "filename": "data.csv"}], self.dest_dir) + self.assertEqual(download_files([], self.dest_dir), []) + finally: + self.env_patcher.start() + + @patch("evaluation_function.s3_files.boto3.client") + def test_missing_s3_object_skipped_others_continue(self, mock_client_factory): + mock_client = MagicMock() + + def fake_head(Bucket, Key): + if Key == "missing.csv": + raise _client_error() + return {"ContentLength": 5} + + mock_client.head_object.side_effect = fake_head + + def fake_download(bucket, key, target): + with open(target, "w") as f: + f.write("ok") + + mock_client.download_file.side_effect = fake_download + mock_client_factory.return_value = mock_client + + files = [ + {"key": "missing.csv", "filename": "missing.csv"}, + {"key": "ok.csv", "filename": "ok.csv"}, + ] + warnings = download_files(files, self.dest_dir) + + self.assertEqual(len(warnings), 1) + self.assertIn("missing.csv", warnings[0]) + self.assertTrue(os.path.exists(os.path.join(self.dest_dir, "ok.csv"))) + + @patch("evaluation_function.s3_files.boto3.client") + def test_download_error_skipped(self, mock_client_factory): + mock_client = MagicMock() + mock_client.head_object.return_value = {"ContentLength": 5} + mock_client.download_file.side_effect = _client_error() + mock_client_factory.return_value = mock_client + + warnings = download_files([{"key": "data.csv", "filename": "data.csv"}], self.dest_dir) + + self.assertEqual(len(warnings), 1) + self.assertIn("data.csv", warnings[0]) + + def test_filename_validation_rejects_traversal(self): + for bad_name in ("../evil.py", "/etc/passwd", "", ".", ".."): + warnings = download_files([{"key": "k", "filename": bad_name}], self.dest_dir) + self.assertEqual(len(warnings), 1, f"expected a warning for filename={bad_name!r}") + + +if __name__ == "__main__": + unittest.main() From d39b96f85a55ea1e9e980c34becbdd6b431e0720 Mon Sep 17 00:00:00 2001 From: Marcus Messer Date: Thu, 13 Aug 2026 10:50:28 +0100 Subject: [PATCH 2/7] Replace S3-based file downloads with HTTPS support Replaces S3 bucket-based file download logic with direct downloads via HTTPS URLs, removing AWS dependency. Updates `evaluation.py`, rewrites `s3_files.py` to handle streaming downloads securely, and adjusts tests and documentation (`CLAUDE.md`, `evaluation_test.py`, `s3_files_test.py`) to reflect the changes. --- CLAUDE.md | 20 ++-- evaluation_function/evaluation.py | 7 +- evaluation_function/evaluation_test.py | 20 ++-- evaluation_function/s3_files.py | 104 +++++++++--------- evaluation_function/s3_files_test.py | 146 ++++++++++++------------- 5 files changed, 148 insertions(+), 149 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 2d2222b..2eb8c38 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -94,17 +94,18 @@ All source lives in `evaluation_function/`: } # 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. +# Downloads files into a per-request working directory (the subprocess's +# cwd) before student code runs, given a pre-signed or public HTTPS URL per +# file (fetched directly with a GET — no AWS credentials needed here). 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. { "mode": "demo", "files": [ - {"key": "uploads//data.csv", "filename": "data.csv"}, - {"key": "uploads//helper.py", "filename": "helper.py"}, + {"url": "https://.../data.csv?X-Amz-Signature=...", "filename": "data.csv"}, + {"url": "https://.../helper.py?X-Amz-Signature=...", "filename": "helper.py"}, ] } ``` @@ -167,8 +168,7 @@ 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 and `files` downloads | -| `S3_FILES_BUCKET` | Runtime env | Bucket name (not URI) that `params["files"]` object keys are resolved against | +| `AWS_*` / boto3 credentials | Runtime env | Required for S3 plot uploads. Not needed for `params["files"]` downloads — those are fetched via plain HTTPS GET from a pre-signed/public URL | Dependencies managed via Poetry; `.venv` is created in-project (`poetry.toml`). diff --git a/evaluation_function/evaluation.py b/evaluation_function/evaluation.py index 398ae64..194517d 100755 --- a/evaluation_function/evaluation.py +++ b/evaluation_function/evaluation.py @@ -11,7 +11,7 @@ from lf_toolkit.evaluation import Result, Params from lf_toolkit.evaluation.image_upload import upload_image, ImageUploadError -from .s3_files import download_files, FileDownloadError +from .s3_files import download_files _TIMEOUT = 25 _UPLOAD_FOLDER = "evaluatePython" @@ -332,10 +332,7 @@ def evaluation_function(response: Any, answer: Any, params: Params) -> Result: 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)] + file_warnings = download_files(file_specs, files_dir) try: if mode == "demo": diff --git a/evaluation_function/evaluation_test.py b/evaluation_function/evaluation_test.py index fc7ee47..f53c3c1 100755 --- a/evaluation_function/evaluation_test.py +++ b/evaluation_function/evaluation_test.py @@ -317,7 +317,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": [{"key": "k", "filename": "data.csv"}]} + params = {"mode": "demo", "files": [{"url": "https://example.com/k", "filename": "data.csv"}]} result = evaluation_function("print(open('data.csv').read())", None, params).to_dict() self.assertIn("1,2,3", result["feedback"]) @@ -327,7 +327,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": [{"key": "k", "filename": "data.csv"}], + "files": [{"url": "https://example.com/k", "filename": "data.csv"}], "tests": [_test("", "42\n"), _test("", "42\n")], } result = evaluation_function("print(open('data.csv').read())", None, params).to_dict() @@ -341,7 +341,7 @@ def test_answer_code_receives_same_files(self, mock_download): params = { "mode": "io_test", "use_answer_as_expected_output": True, - "files": [{"key": "k", "filename": "data.csv"}], + "files": [{"url": "https://example.com/k", "filename": "data.csv"}], "tests": [{"input": ""}], } code = "print(open('data.csv').read())" @@ -352,7 +352,7 @@ 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": [{"key": "k", "filename": "data.csv"}]} + params = {"mode": "demo", "files": [{"url": "https://example.com/k", "filename": "data.csv"}]} result = evaluation_function("print('hi')", None, params).to_dict() self.assertIn("could not be found", result["feedback"]) @@ -360,7 +360,7 @@ def test_missing_file_reported_as_warning(self, mock_download): @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"}]} + params = {"mode": "demo", "files": [{"url": "https://example.com/k", "filename": "helper.py"}]} result = evaluation_function("import helper\nprint(helper.square(4))", None, params).to_dict() self.assertIn("16", result["feedback"]) @@ -376,7 +376,7 @@ 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"}]} + params = {"mode": "demo", "files": [{"url": "https://example.com/k", "filename": "data.csv"}]} result = evaluation_function("print(open('data.csv').read())", None, params).to_dict() self.assertIn("hello", result["feedback"]) @@ -384,7 +384,7 @@ def test_read_downloaded_file_succeeds(self, mock_download): @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"}]} + params = {"mode": "demo", "files": [{"url": "https://example.com/k", "filename": "data.csv"}]} result = evaluation_function("open('data.csv', 'w')", None, params).to_dict() self.assertIn("read-only", result["feedback"]) @@ -392,7 +392,7 @@ def test_write_mode_to_provided_file_blocked(self, mock_download): @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"}]} + params = {"mode": "demo", "files": [{"url": "https://example.com/k", "filename": "data.csv"}]} result = evaluation_function("open('output.txt', 'w')", None, params).to_dict() self.assertIn("read-only", result["feedback"]) @@ -400,7 +400,7 @@ def test_write_new_file_in_run_dir_blocked(self, mock_download): @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"}]} + params = {"mode": "demo", "files": [{"url": "https://example.com/k", "filename": "data.csv"}]} code = "from pathlib import Path\nprint(Path('data.csv').read_text())" result = evaluation_function(code, None, params).to_dict() @@ -409,7 +409,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": [{"key": "k", "filename": "data.csv"}]} + params = {"mode": "demo", "files": [{"url": "https://example.com/k", "filename": "data.csv"}]} code = "from pathlib import Path\nPath('data.csv').write_text('nope')" result = evaluation_function(code, None, params).to_dict() diff --git a/evaluation_function/s3_files.py b/evaluation_function/s3_files.py index 0c61065..b5b73d1 100644 --- a/evaluation_function/s3_files.py +++ b/evaluation_function/s3_files.py @@ -1,94 +1,92 @@ import os from typing import TypedDict +from urllib.parse import urlparse -import boto3 -from botocore.config import Config -from botocore.exceptions import ClientError +import requests _MAX_FILE_BYTES = 5 * 1024 * 1024 _MAX_TOTAL_BYTES = 20 * 1024 * 1024 _DOWNLOAD_TIMEOUT = 10 +_CHUNK_SIZE = 65536 class FileSpec(TypedDict): - key: str + url: str filename: str -class FileDownloadError(Exception): - """Raised for whole-request configuration problems (e.g. missing bucket env var).""" - pass - +def _valid_filename(filename: str) -> bool: + if not filename or filename in (".", ".."): + return False + return os.path.basename(filename) == filename -def _s3_client(): - return boto3.client( - "s3", - region_name=os.environ.get("AWS_REGION", "eu-west-2"), - config=Config( - connect_timeout=_DOWNLOAD_TIMEOUT, - read_timeout=_DOWNLOAD_TIMEOUT, - retries={"max_attempts": 2}, - ), - ) +class _FileTooLarge(Exception): + pass -def _get_bucket_name() -> str: - bucket = os.environ.get("S3_FILES_BUCKET") - if not bucket: - raise FileDownloadError("S3_FILES_BUCKET environment variable is not set") - return bucket +def _download_one(url: str, target: str, remaining_budget: int) -> int: + """Stream url into target. Returns bytes written. -def _valid_filename(filename: str) -> bool: - if not filename or filename in (".", ".."): - return False - return os.path.basename(filename) == filename + Raises _FileTooLarge (and removes any partial file) if the download + exceeds _MAX_FILE_BYTES or remaining_budget, or requests.RequestException + for network/HTTP errors — both handled by the caller. + """ + resp = requests.get(url, stream=True, timeout=_DOWNLOAD_TIMEOUT) + resp.raise_for_status() + + content_length = resp.headers.get("Content-Length") + cap = min(_MAX_FILE_BYTES, remaining_budget) + if content_length is not None and int(content_length) > cap: + raise _FileTooLarge() + + written = 0 + try: + with open(target, "wb") as f: + for chunk in resp.iter_content(chunk_size=_CHUNK_SIZE): + written += len(chunk) + if written > cap: + raise _FileTooLarge() + f.write(chunk) + except _FileTooLarge: + if os.path.exists(target): + os.unlink(target) + raise + return written def download_files(files: list[FileSpec], dest_dir: str) -> list[str]: """Download each file into dest_dir. - Returns a list of warning strings for files that were skipped (missing, - too large, or errored) — never raises for per-file problems, only for - whole-config problems (missing bucket env var). + Returns a list of warning strings for files that were skipped (invalid + filename/URL, too large, or errored) — never raises. """ if not files: return [] - bucket = _get_bucket_name() - client = _s3_client() - + real_dest_dir = os.path.realpath(dest_dir) warnings: list[str] = [] total_bytes = 0 for spec in files: - key = spec["key"] + url = spec["url"] filename = spec["filename"] if not _valid_filename(filename): warnings.append(f"File '{filename}' has an invalid filename and was not made available.") continue - real_dest_dir = os.path.realpath(dest_dir) target = os.path.realpath(os.path.join(real_dest_dir, filename)) if os.path.commonpath([target, real_dest_dir]) != real_dest_dir: warnings.append(f"File '{filename}' has an invalid filename and was not made available.") continue - try: - head = client.head_object(Bucket=bucket, Key=key) - except ClientError as e: - warnings.append(f"File '{filename}' could not be found or accessed ({e}).") + if urlparse(url).scheme != "https": + warnings.append(f"File '{filename}' has an invalid URL and was not made available.") continue - size = head.get("ContentLength", 0) - if size > _MAX_FILE_BYTES: - warnings.append( - f"File '{filename}' exceeds the {_MAX_FILE_BYTES // (1024 * 1024)}MB size limit " - "and was not made available." - ) - continue - if total_bytes + size > _MAX_TOTAL_BYTES: + remaining_budget = _MAX_TOTAL_BYTES - total_bytes + if remaining_budget <= 0: warnings.append( f"File '{filename}' was skipped because it would exceed the total " f"{_MAX_TOTAL_BYTES // (1024 * 1024)}MB size limit for this run." @@ -96,11 +94,17 @@ def download_files(files: list[FileSpec], dest_dir: str) -> list[str]: continue try: - client.download_file(bucket, key, target) - except ClientError as e: + written = _download_one(url, target, remaining_budget) + except _FileTooLarge: + warnings.append( + f"File '{filename}' exceeds the {_MAX_FILE_BYTES // (1024 * 1024)}MB size limit " + "and was not made available." + ) + continue + except requests.exceptions.RequestException as e: warnings.append(f"File '{filename}' could not be downloaded ({e}).") continue - total_bytes += size + total_bytes += written return warnings diff --git a/evaluation_function/s3_files_test.py b/evaluation_function/s3_files_test.py index addd0e1..af53501 100644 --- a/evaluation_function/s3_files_test.py +++ b/evaluation_function/s3_files_test.py @@ -3,105 +3,101 @@ import unittest from unittest.mock import MagicMock, patch -from botocore.exceptions import ClientError +import requests -from .s3_files import download_files, FileDownloadError, _MAX_FILE_BYTES +from .s3_files import download_files, _MAX_FILE_BYTES +_URL = "https://example-bucket.s3.amazonaws.com/data.csv?X-Amz-Signature=abc" -def _client_error(): - return ClientError({"Error": {"Code": "404", "Message": "Not Found"}}, "HeadObject") + +def _fake_response(content: bytes, content_length: int | None = None, status_code: int = 200): + resp = MagicMock() + resp.status_code = status_code + resp.headers = {} + if content_length is not None: + resp.headers["Content-Length"] = str(content_length) + + def raise_for_status(): + if status_code >= 400: + raise requests.exceptions.HTTPError(f"{status_code} error") + + resp.raise_for_status.side_effect = raise_for_status + + chunk_size = 65536 + + def iter_content(chunk_size=chunk_size): + for i in range(0, len(content), chunk_size): + yield content[i:i + chunk_size] + + resp.iter_content.side_effect = iter_content + return resp class TestDownloadFiles(unittest.TestCase): def setUp(self): self.dest_dir = tempfile.mkdtemp() - self.env_patcher = patch.dict(os.environ, {"S3_FILES_BUCKET": "test-bucket"}) - self.env_patcher.start() - def tearDown(self): - self.env_patcher.stop() + def test_no_files_returns_empty(self): + self.assertEqual(download_files([], self.dest_dir), []) - def test_no_files_returns_empty_without_client(self): - warnings = download_files([], self.dest_dir) - self.assertEqual(warnings, []) + @patch("evaluation_function.s3_files.requests.get") + def test_successful_download_writes_file(self, mock_get): + mock_get.return_value = _fake_response(b"hello", content_length=5) - @patch("evaluation_function.s3_files.boto3.client") - def test_successful_download_writes_file(self, mock_client_factory): - mock_client = MagicMock() - mock_client.head_object.return_value = {"ContentLength": 10} + warnings = download_files([{"url": _URL, "filename": "data.csv"}], self.dest_dir) - def fake_download(bucket, key, target): - with open(target, "w") as f: - f.write("hello") + self.assertEqual(warnings, []) + with open(os.path.join(self.dest_dir, "data.csv"), "rb") as f: + self.assertEqual(f.read(), b"hello") - mock_client.download_file.side_effect = fake_download - mock_client_factory.return_value = mock_client + @patch("evaluation_function.s3_files.requests.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([{"key": "data.csv", "filename": "data.csv"}], self.dest_dir) + warnings = download_files([{"url": _URL, "filename": "big.csv"}], self.dest_dir) - self.assertEqual(warnings, []) - with open(os.path.join(self.dest_dir, "data.csv")) as f: - self.assertEqual(f.read(), "hello") + self.assertEqual(len(warnings), 1) + self.assertIn("big.csv", warnings[0]) + self.assertFalse(os.path.exists(os.path.join(self.dest_dir, "big.csv"))) - @patch("evaluation_function.s3_files.boto3.client") - def test_oversized_file_skipped(self, mock_client_factory): - mock_client = MagicMock() - mock_client.head_object.return_value = {"ContentLength": _MAX_FILE_BYTES + 1} - mock_client_factory.return_value = mock_client + @patch("evaluation_function.s3_files.requests.get") + def test_oversized_via_streaming_skipped(self, mock_get): + # Content-Length lies (claims small), actual streamed bytes exceed the cap. + big_content = b"x" * (_MAX_FILE_BYTES + 1) + mock_get.return_value = _fake_response(big_content, content_length=10) - warnings = download_files([{"key": "big.csv", "filename": "big.csv"}], self.dest_dir) + warnings = download_files([{"url": _URL, "filename": "big.csv"}], self.dest_dir) self.assertEqual(len(warnings), 1) self.assertIn("big.csv", warnings[0]) - mock_client.download_file.assert_not_called() self.assertFalse(os.path.exists(os.path.join(self.dest_dir, "big.csv"))) - @patch("evaluation_function.s3_files.boto3.client") - def test_total_size_cap_skips_later_files(self, mock_client_factory): + @patch("evaluation_function.s3_files.requests.get") + def test_total_size_cap_skips_later_files(self, mock_get): # 5 files at exactly the per-file cap: the first 4 sum to exactly - # _MAX_TOTAL_BYTES (allowed), the 5th would push over it (skipped). - mock_client = MagicMock() - mock_client.head_object.return_value = {"ContentLength": _MAX_FILE_BYTES} - mock_client_factory.return_value = mock_client + # _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 = [{"key": f"{i}.csv", "filename": f"{i}.csv"} for i in range(5)] + files = [{"url": _URL, "filename": f"{i}.csv"} for i in range(5)] warnings = download_files(files, self.dest_dir) - self.assertEqual(mock_client.download_file.call_count, 4) + self.assertEqual(mock_get.call_count, 4) self.assertEqual(len(warnings), 1) self.assertIn("4.csv", warnings[0]) - def test_missing_bucket_env_var_raises_only_when_files_present(self): - self.env_patcher.stop() - try: - with self.assertRaises(FileDownloadError): - download_files([{"key": "data.csv", "filename": "data.csv"}], self.dest_dir) - self.assertEqual(download_files([], self.dest_dir), []) - finally: - self.env_patcher.start() - - @patch("evaluation_function.s3_files.boto3.client") - def test_missing_s3_object_skipped_others_continue(self, mock_client_factory): - mock_client = MagicMock() + @patch("evaluation_function.s3_files.requests.get") + def test_http_error_skipped_others_continue(self, mock_get): + def side_effect(url, stream, timeout): + if url == "https://example.com/missing": + return _fake_response(b"", status_code=404) + return _fake_response(b"ok", content_length=2) - def fake_head(Bucket, Key): - if Key == "missing.csv": - raise _client_error() - return {"ContentLength": 5} - - mock_client.head_object.side_effect = fake_head - - def fake_download(bucket, key, target): - with open(target, "w") as f: - f.write("ok") - - mock_client.download_file.side_effect = fake_download - mock_client_factory.return_value = mock_client + mock_get.side_effect = side_effect files = [ - {"key": "missing.csv", "filename": "missing.csv"}, - {"key": "ok.csv", "filename": "ok.csv"}, + {"url": "https://example.com/missing", "filename": "missing.csv"}, + {"url": "https://example.com/ok", "filename": "ok.csv"}, ] warnings = download_files(files, self.dest_dir) @@ -109,21 +105,23 @@ def fake_download(bucket, key, target): self.assertIn("missing.csv", warnings[0]) self.assertTrue(os.path.exists(os.path.join(self.dest_dir, "ok.csv"))) - @patch("evaluation_function.s3_files.boto3.client") - def test_download_error_skipped(self, mock_client_factory): - mock_client = MagicMock() - mock_client.head_object.return_value = {"ContentLength": 5} - mock_client.download_file.side_effect = _client_error() - mock_client_factory.return_value = mock_client + @patch("evaluation_function.s3_files.requests.get") + def test_network_error_skipped(self, mock_get): + mock_get.side_effect = requests.exceptions.ConnectionError("boom") - warnings = download_files([{"key": "data.csv", "filename": "data.csv"}], self.dest_dir) + warnings = download_files([{"url": _URL, "filename": "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) + 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([{"key": "k", "filename": bad_name}], self.dest_dir) + 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}") From adcd99ab1251ac705ad60dbdf558584905198b4a Mon Sep 17 00:00:00 2001 From: Marcus Messer Date: Tue, 18 Aug 2026 09:00:05 +0100 Subject: [PATCH 3/7] Update Dockerfile to use the latest Python base image Switches from a version-specific base image to the `latest` tag for the evaluation function, ensuring up-to-date dependencies and compatibility. --- Dockerfile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Dockerfile b/Dockerfile index 371fa08..19e9ed4 100755 --- a/Dockerfile +++ b/Dockerfile @@ -1,4 +1,4 @@ -FROM ghcr.io/lambda-feedback/evaluation-function-base/python:test-sandbox-3.12 AS builder +FROM ghcr.io/lambda-feedback/evaluation-function-base/python:latest AS builder RUN pip install poetry==1.8.3 @@ -12,7 +12,7 @@ COPY pyproject.toml poetry.lock ./ RUN --mount=type=cache,target=$POETRY_CACHE_DIR \ poetry install --without dev --no-root -FROM ghcr.io/lambda-feedback/evaluation-function-base/python:test-sandbox-3.12 +FROM ghcr.io/lambda-feedback/evaluation-function-base/python:latest ENV VIRTUAL_ENV=/app/.venv \ PATH="/app/.venv/bin:$PATH" From 208bf574192c9ccb87e0c0c55deb80c0b634a4e4 Mon Sep 17 00:00:00 2001 From: Marcus Messer Date: Tue, 18 Aug 2026 09:38:33 +0100 Subject: [PATCH 4/7] 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. --- CLAUDE.md | 4 +- evaluation_function/evaluation.py | 48 +++++++++------ evaluation_function/evaluation_test.py | 82 ++++++++++++++++++++++---- evaluation_function/s3_files.py | 23 +++++++- evaluation_function/s3_files_test.py | 57 ++++++++++++++---- 5 files changed, 170 insertions(+), 44 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 2eb8c38..cbcdaf3 100644 --- a/CLAUDE.md +++ b/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"}, ] } ``` diff --git a/evaluation_function/evaluation.py b/evaluation_function/evaluation.py index 194517d..b77d045 100755 --- a/evaluation_function/evaluation.py +++ b/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 \ No newline at end of file diff --git a/evaluation_function/evaluation_test.py b/evaluation_function/evaluation_test.py index f53c3c1..63fe3be 100755 --- a/evaluation_function/evaluation_test.py +++ b/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,7 +353,7 @@ 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"]) @@ -360,7 +361,7 @@ def test_missing_file_reported_as_warning(self, mock_download): @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,12 +372,73 @@ 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"]) @@ -384,7 +446,7 @@ def test_read_downloaded_file_succeeds(self, mock_download): @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"]) @@ -392,7 +454,7 @@ def test_write_mode_to_provided_file_blocked(self, mock_download): @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"]) @@ -400,7 +462,7 @@ def test_write_new_file_in_run_dir_blocked(self, mock_download): @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() diff --git a/evaluation_function/s3_files.py b/evaluation_function/s3_files.py index b5b73d1..169c19e 100644 --- a/evaluation_function/s3_files.py +++ b/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.") diff --git a/evaluation_function/s3_files_test.py b/evaluation_function/s3_files_test.py index af53501..1ec27d4 100644 --- a/evaluation_function/s3_files_test.py +++ b/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__": From 0aed5f1db84ae3da50521335233c0e5395d02ba0 Mon Sep 17 00:00:00 2001 From: Marcus Messer Date: Wed, 9 Sep 2026 18:33:49 +0100 Subject: [PATCH 5/7] Support file uploads via response payload and prioritize over `params["files"]` Adds `_resolve_submission` to handle responses containing code and file specifications in `"code"` and `"files"` keys. Updates `evaluation.py` to normalize and prioritize file sources from the response payload over `params["files"]`. Enhances tests to validate this logic. Updates documentation to reflect the new behavior. --- CLAUDE.md | 13 +++- evaluation_function/evaluation.py | 59 +++++++++++++++-- evaluation_function/evaluation_test.py | 87 ++++++++++++++++++++++++++ 3 files changed, 153 insertions(+), 6 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index cbcdaf3..3e0d88b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -17,7 +17,7 @@ All source lives in `evaluation_function/`: ### Evaluation pipeline (`evaluation.py`) 1. Run AST security check on student code -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 +2. Resolve the submission into `(code, file_specs)` via `_resolve_submission`: the response may be a bare code string, or a `{"code", "files"}` object (or JSON string of one) as sent by the LF web client's upload widget. If `file_specs` (from `response["files"]`, else `params["files"]`) is non-empty, download the listed 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 @@ -108,6 +108,17 @@ All source lives in `evaluation_function/`: {"url": "https://.../helper.py?X-Amz-Signature=...", "name": "helper.py"}, ] } + +# files in the response payload (how the LF web client sends uploads) +# When the response area has a file-upload widget, the client delivers the +# submission as {"code": ..., "files": [...]} (sometimes as a JSON string of +# that object), with each file entry itself possibly a JSON string. +# evaluation_function unpacks this: response["code"] becomes the student +# code, response["files"] becomes the file list. Files in the response take +# precedence over params["files"], which stays as a fallback. Entry shape is +# the same {"url", "name"} as params["files"]. +{"code": "print(open('data.csv').read())", + "files": [{"url": "https://.../data.csv?...", "name": "data.csv"}]} ``` ### Security model (`preview.py`) diff --git a/evaluation_function/evaluation.py b/evaluation_function/evaluation.py index b77d045..27151ce 100755 --- a/evaluation_function/evaluation.py +++ b/evaluation_function/evaluation.py @@ -322,6 +322,54 @@ def _evaluate_unit(response: str, test_code: str, result: Result, files_dir: str return result +def _coerce_file_specs(raw: Any) -> list: + """Normalise a raw files value into a list of {url, name} dicts. + + Entries may already be dicts, or JSON-encoded strings — the LF web + client currently serialises each upload entry to a string. + """ + if not isinstance(raw, (list, tuple)): + return [] + specs = [] + for entry in raw: + if isinstance(entry, str): + try: + entry = json.loads(entry) + except (ValueError, TypeError): + continue + if isinstance(entry, dict): + specs.append(entry) + return specs + + +def _resolve_submission(response: Any, params: Params) -> tuple[str, list]: + """Split the submission into (code, file_specs). + + When file upload is enabled, the LF web client delivers the response + payload as {"code": ..., "files": [...]} (sometimes as a JSON string of + that object) rather than a bare code string. Files listed in the + response take precedence; params["files"] is the fallback. + """ + payload = response + if isinstance(payload, str): + try: + parsed = json.loads(payload) + except (ValueError, TypeError): + parsed = None + if isinstance(parsed, dict) and ("code" in parsed or "files" in parsed): + payload = parsed + + if isinstance(payload, dict): + code = payload.get("code") or "" + response_files = _coerce_file_specs(payload.get("files")) + else: + code = payload if isinstance(payload, str) else str(payload) + response_files = [] + + file_specs = response_files or _coerce_file_specs(params.get("files")) + return str(code), file_specs + + def evaluation_function(response: Any, answer: Any, params: Params) -> Result: result = Result() mode = params.get("mode") @@ -329,22 +377,23 @@ 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 + code, file_specs = _resolve_submission(response, params) + files_dir = None 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) + result = _evaluate_demo(code, 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) + result = _evaluate_io(code, 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) + result = _evaluate_unit(code, test_code, result, files_dir=files_dir) for warning in file_warnings: result.add_feedback("error", warning) @@ -352,7 +401,7 @@ def evaluation_function(response: Any, answer: Any, params: Params) -> Result: 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) + violations = _check_pep8(code, select) if violations: body = "Style suggestions (PEP8):\n" + "\n".join(f"- {v}" for v in violations) else: diff --git a/evaluation_function/evaluation_test.py b/evaluation_function/evaluation_test.py index 63fe3be..165910b 100755 --- a/evaluation_function/evaluation_test.py +++ b/evaluation_function/evaluation_test.py @@ -1,3 +1,4 @@ +import json import os import tempfile import unittest @@ -392,6 +393,92 @@ def test_legacy_filename_key_does_not_crash(self): self.assertIn("missing", result["feedback"].lower()) +class TestFilesInResponsePayload(unittest.TestCase): + """The LF web client delivers uploads inside the response payload as + {"code": ..., "files": [...]} rather than in params["files"].""" + + @patch("evaluation_function.evaluation.download_files") + def test_response_dict_with_code_and_files(self, mock_download): + mock_download.side_effect = _stub_download({"data.csv": "1,2,3"}) + response = { + "code": "print(open('data.csv').read())", + "files": [{"url": "https://example.com/k", "name": "data.csv"}], + } + result = evaluation_function(response, None, {"mode": "demo"}).to_dict() + + self.assertIn("1,2,3", result["feedback"]) + mock_download.assert_called_once() + passed_specs = mock_download.call_args[0][0] + self.assertEqual(passed_specs, [{"url": "https://example.com/k", "name": "data.csv"}]) + + @patch("evaluation_function.evaluation.download_files") + def test_response_dict_file_entries_are_json_strings(self, mock_download): + mock_download.side_effect = _stub_download({"data.csv": "42"}) + response = { + "code": "print(open('data.csv').read())", + "files": [json.dumps({"url": "https://example.com/k", "name": "data.csv"})], + } + result = evaluation_function(response, None, {"mode": "demo"}).to_dict() + + self.assertIn("42", result["feedback"]) + passed_specs = mock_download.call_args[0][0] + self.assertEqual(passed_specs, [{"url": "https://example.com/k", "name": "data.csv"}]) + + @patch("evaluation_function.evaluation.download_files") + def test_response_is_json_string_of_payload(self, mock_download): + mock_download.side_effect = _stub_download({"data.csv": "7"}) + response = json.dumps({ + "code": "print(open('data.csv').read())", + "files": [{"url": "https://example.com/k", "name": "data.csv"}], + }) + result = evaluation_function(response, None, {"mode": "demo"}).to_dict() + + self.assertIn("7", result["feedback"]) + mock_download.assert_called_once() + + @patch("evaluation_function.evaluation.download_files") + def test_unit_test_mode_reads_files_from_response(self, mock_download): + mock_download.side_effect = _stub_download({"data.csv": "x"}) + response = { + "code": "", + "files": [{"url": "https://example.com/k", "name": "data.csv"}], + } + params = { + "mode": "unit_test", + "test_code": "import os\ndef test_present():\n assert os.path.isfile('data.csv')\n", + } + result = evaluation_function(response, None, params).to_dict() + + self.assertTrue(result["is_correct"]) + self.assertIn("1/1 tests passed", result["feedback"]) + + @patch("evaluation_function.evaluation.download_files") + def test_plain_string_response_still_uses_params_files(self, mock_download): + mock_download.side_effect = _stub_download({"data.csv": "9"}) + 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("9", result["feedback"]) + + @patch("evaluation_function.evaluation.download_files") + def test_response_files_take_precedence_over_params_files(self, mock_download): + mock_download.side_effect = _stub_download({"data.csv": "from_response"}) + response = { + "code": "print(open('data.csv').read())", + "files": [{"url": "https://example.com/response", "name": "data.csv"}], + } + params = {"mode": "demo", "files": [{"url": "https://example.com/params", "name": "other.csv"}]} + evaluation_function(response, None, params) + + passed_specs = mock_download.call_args[0][0] + self.assertEqual(passed_specs, [{"url": "https://example.com/response", "name": "data.csv"}]) + + def test_response_dict_without_files_no_download(self): + with patch("evaluation_function.evaluation.download_files") as mock_download: + evaluation_function({"code": "print('hi')"}, None, {"mode": "demo"}) + mock_download.assert_not_called() + + class TestUnexpectedExceptionHandling(unittest.TestCase): @patch("evaluation_function.evaluation._run_code") From abb596c26cdebb403d0a80363b64dba73fdefdf1 Mon Sep 17 00:00:00 2001 From: Marcus Messer Date: Wed, 9 Sep 2026 19:16:16 +0100 Subject: [PATCH 6/7] Add support for unwrapping `{"code", "files"}` payloads in answers and submissions Refactors `_resolve_submission` into `_unwrap_payload` and updates answer handling to support the `{"code", "files"}` structure. Adds unit tests for evaluation behavior with structured answers. --- evaluation_function/evaluation.py | 43 +++++++++++++++++--------- evaluation_function/evaluation_test.py | 36 +++++++++++++++++++++ 2 files changed, 64 insertions(+), 15 deletions(-) diff --git a/evaluation_function/evaluation.py b/evaluation_function/evaluation.py index b527de4..30f51b4 100755 --- a/evaluation_function/evaluation.py +++ b/evaluation_function/evaluation.py @@ -343,15 +343,15 @@ def _coerce_file_specs(raw: Any) -> list: return specs -def _resolve_submission(response: Any, params: Params) -> tuple[str, list]: - """Split the submission into (code, file_specs). +def _unwrap_payload(value: Any) -> tuple[str, list]: + """Return (code, file_specs) from a submission or answer value. - When file upload is enabled, the LF web client delivers the response - payload as {"code": ..., "files": [...]} (sometimes as a JSON string of - that object) rather than a bare code string. Files listed in the - response take precedence; params["files"] is the fallback. + When file upload is enabled, the LF web client delivers the value as + {"code": ..., "files": [...]} (sometimes as a JSON string of that + object) rather than a bare code string. A plain string is returned + unchanged with no files. """ - payload = response + payload = value if isinstance(payload, str): try: parsed = json.loads(payload) @@ -361,14 +361,27 @@ def _resolve_submission(response: Any, params: Params) -> tuple[str, list]: payload = parsed if isinstance(payload, dict): - code = payload.get("code") or "" - response_files = _coerce_file_specs(payload.get("files")) - else: - code = payload if isinstance(payload, str) else str(payload) - response_files = [] + return str(payload.get("code") or ""), _coerce_file_specs(payload.get("files")) + if isinstance(payload, str): + return payload, [] + return str(payload), [] + +def _resolve_submission(response: Any, params: Params) -> tuple[str, list]: + """Split the submission into (code, file_specs). + + Files listed in the response take precedence; params["files"] is the + fallback. + """ + code, response_files = _unwrap_payload(response) file_specs = response_files or _coerce_file_specs(params.get("files")) - return str(code), file_specs + return code, file_specs + + +def _answer_code(answer: Any) -> str: + """The code string from the answer field, unwrapping a {code, files} + payload the same way the submission is unwrapped.""" + return _unwrap_payload(answer)[0] def evaluation_function(response: Any, answer: Any, params: Params) -> Result: @@ -398,10 +411,10 @@ def evaluation_function(response: Any, answer: Any, params: Params) -> Result: if mode == "demo": result = _evaluate_demo(code, result, files_dir) elif mode == "io_test": - ans = str(answer) if params.get("use_answer_as_expected_output") else "" + ans = _answer_code(answer) if params.get("use_answer_as_expected_output") else "" result = _evaluate_io(code, 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", "") + test_code = _answer_code(answer) if params.get("use_answer_as_test_code") else params.get("test_code", "") result = _evaluate_unit(code, test_code, result, files_dir=files_dir) for warning in file_warnings: diff --git a/evaluation_function/evaluation_test.py b/evaluation_function/evaluation_test.py index 663c643..0bdd549 100755 --- a/evaluation_function/evaluation_test.py +++ b/evaluation_function/evaluation_test.py @@ -479,6 +479,42 @@ def test_response_dict_without_files_no_download(self): mock_download.assert_not_called() +class TestAnswerFieldPayload(unittest.TestCase): + """With the file-upload widget the answer field is delivered in the same + {"code", "files"} shape as the submission, not as a bare string.""" + + def test_unit_test_code_from_answer_dict(self): + response = {"code": "def square(n):\n return n * n\n"} + answer = {"code": "def test_sq():\n assert square(4) == 16\n", "files": []} + params = {"mode": "unit_test", "use_answer_as_test_code": True} + result = evaluation_function(response, answer, params).to_dict() + + self.assertTrue(result["is_correct"]) + self.assertIn("1/1 tests passed", result["feedback"]) + + def test_unit_test_code_from_answer_json_string(self): + answer = json.dumps({"code": "def test_ok():\n assert True\n", "files": []}) + params = {"mode": "unit_test", "use_answer_as_test_code": True} + result = evaluation_function({"code": ""}, answer, params).to_dict() + + self.assertIn("1/1 tests passed", result["feedback"]) + + def test_io_expected_output_from_answer_dict(self): + response = {"code": "print(6)"} + answer = {"code": "print(2 * 3)", "files": []} + params = {"mode": "io_test", "use_answer_as_expected_output": True, + "tests": [{"input": ""}]} + result = evaluation_function(response, answer, params).to_dict() + + self.assertTrue(result["is_correct"]) + + def test_plain_string_answer_still_works(self): + params = {"mode": "unit_test", "use_answer_as_test_code": True} + result = evaluation_function("x = 1", "def test_ok():\n assert True\n", params).to_dict() + + self.assertIn("1/1 tests passed", result["feedback"]) + + class TestUnexpectedExceptionHandling(unittest.TestCase): @patch("evaluation_function.evaluation._run_code") From ee80ab954c8c10a97e43132dc6ca599672c9b772 Mon Sep 17 00:00:00 2001 From: Marcus Messer Date: Thu, 24 Sep 2026 16:46:27 +0100 Subject: [PATCH 7/7] Refactor file-handling logic to unify `answer_files` and `response_files` handling Replaces `_resolve_submission` and `_unwrap_payload` with `_collect_file_specs` to streamline file normalization and prioritization. Updates test suite to validate the new logic and modifies documentation to reflect the updated file specification and submission flow. --- CLAUDE.md | 44 +++---- evaluation_function/evaluation.py | 57 +++------ evaluation_function/evaluation_test.py | 168 +++++++------------------ 3 files changed, 82 insertions(+), 187 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index c6ca79a..42261c9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -11,13 +11,13 @@ 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 → plot upload (GCS/S3 via lf_toolkit) → 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 | +| `s3_files.py` | Downloads `params["answer_files"]`/`params["response_files"]` objects 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. Resolve the submission into `(code, file_specs)` via `_resolve_submission`: the response may be a bare code string, or a `{"code", "files"}` object (or JSON string of one) as sent by the LF web client's upload widget. If `file_specs` (from `response["files"]`, else `params["files"]`) is non-empty, download the listed objects once into a per-request working directory (see `s3_files.py`), used as the subprocess `cwd` for every run in this request +2. Gather file specs from params via `_collect_file_specs`: `params["answer_files"]` (teacher, plus legacy `params["files"]`) and `params["response_files"]` (student); teacher files win on a name clash. The response and answer are plain code strings. If any files are listed, download the listed 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 @@ -93,32 +93,28 @@ All source lives in `evaluation_function/`: "tests": [...] } -# files — optional, works with all modes -# Downloads files into a per-request working directory (the subprocess's -# cwd) before student code runs, given a pre-signed or public HTTPS URL per -# file (fetched directly with a GET — no AWS credentials needed here). 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. +# answer_files / response_files — optional, work with all modes +# answer_files: the teacher's files, saved in the response area's gradeParams. +# response_files: the student's uploads, sent with each check as additionalParams. +# params["files"] is still accepted as a legacy alias for answer_files. +# All listed files are downloaded into one per-request working directory +# (the subprocess's cwd) before student code runs, given a pre-signed or +# public HTTPS URL per file (fetched directly with a GET — no AWS +# credentials needed here). On a name clash the teacher's file wins. Entries +# may be dicts or JSON strings of dicts. Data files can be read with +# open()/pandas.read_csv()/etc.; .py files are importable 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. { "mode": "demo", - "files": [ + "answer_files": [ {"url": "https://.../data.csv?X-Amz-Signature=...", "name": "data.csv"}, {"url": "https://.../helper.py?X-Amz-Signature=...", "name": "helper.py"}, + ], + "response_files": [ + {"url": "https://.../mine.csv?X-Amz-Signature=...", "name": "mine.csv"}, ] } - -# files in the response payload (how the LF web client sends uploads) -# When the response area has a file-upload widget, the client delivers the -# submission as {"code": ..., "files": [...]} (sometimes as a JSON string of -# that object), with each file entry itself possibly a JSON string. -# evaluation_function unpacks this: response["code"] becomes the student -# code, response["files"] becomes the file list. Files in the response take -# precedence over params["files"], which stays as a fallback. Entry shape is -# the same {"url", "name"} as params["files"]. -{"code": "print(open('data.csv').read())", - "files": [{"url": "https://.../data.csv?...", "name": "data.csv"}]} ``` ### Security model (`preview.py`) @@ -129,7 +125,7 @@ All source lives in `evaluation_function/`: - **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. +`open`/`pathlib` are intentionally **not** blocked here — they're needed to read files loaded via `params["answer_files"]`/`params["response_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 @@ -184,7 +180,7 @@ CI runs on Python 3.12 and uploads JUnit XML results (`.github/workflows/test-li | `LOG_LEVEL` | `debug` | Logging verbosity | | `IMAGE_UPLOAD_BACKEND` | `gcs` | Plot upload backend in lf_toolkit (`gcs` set in Dockerfile; override to `s3` on the service to use AWS) | | `GCS_BUCKET` | Runtime env | Target bucket for matplotlib plot uploads; set per-environment on the Cloud Run service. Auth is via the runtime service account (ADC) — no keys | -| `AWS_*` / `S3_BUCKET_URI` | Runtime env | Only for the legacy S3 plot-upload backend (`IMAGE_UPLOAD_BACKEND=s3`). Not needed for `params["files"]` / response-payload file downloads — those are plain HTTPS GETs from a pre-signed/public URL | +| `AWS_*` / `S3_BUCKET_URI` | Runtime env | Only for the legacy S3 plot-upload backend (`IMAGE_UPLOAD_BACKEND=s3`). Not needed for `answer_files` / `response_files` downloads — those are plain HTTPS GETs from a pre-signed/public URL | | `SANDBOX_ENABLED` | `true` | Wrap the worker in shimmy's nsjail sandbox (needs `--privileged` at run time) | | `SANDBOX_SECCOMP` | `true` | nsjail seccomp syscall filter | | `SANDBOX_RO_BINDS` | `/usr:/lib:/lib64:/bin:/sbin:/etc:/app` | Read-only bind mounts visible inside the jail | diff --git a/evaluation_function/evaluation.py b/evaluation_function/evaluation.py index 30f51b4..60b61e4 100755 --- a/evaluation_function/evaluation.py +++ b/evaluation_function/evaluation.py @@ -327,7 +327,7 @@ def _coerce_file_specs(raw: Any) -> list: """Normalise a raw files value into a list of {url, name} dicts. Entries may already be dicts, or JSON-encoded strings — the LF web - client currently serialises each upload entry to a string. + client may serialise each upload entry to a string. """ if not isinstance(raw, (list, tuple)): return [] @@ -343,45 +343,19 @@ def _coerce_file_specs(raw: Any) -> list: return specs -def _unwrap_payload(value: Any) -> tuple[str, list]: - """Return (code, file_specs) from a submission or answer value. +def _collect_file_specs(params: Params) -> list: + """Gather the files to make available for this request. - When file upload is enabled, the LF web client delivers the value as - {"code": ..., "files": [...]} (sometimes as a JSON string of that - object) rather than a bare code string. A plain string is returned - unchanged with no files. + The LF client passes the teacher's files as params["answer_files"] + (saved in the response area's grade params) and the student's uploads + as params["response_files"] (sent with each check). params["files"] is + accepted as a legacy alias for answer_files. All files land in one + working directory; on a name clash the teacher's file wins. """ - payload = value - if isinstance(payload, str): - try: - parsed = json.loads(payload) - except (ValueError, TypeError): - parsed = None - if isinstance(parsed, dict) and ("code" in parsed or "files" in parsed): - payload = parsed - - if isinstance(payload, dict): - return str(payload.get("code") or ""), _coerce_file_specs(payload.get("files")) - if isinstance(payload, str): - return payload, [] - return str(payload), [] - - -def _resolve_submission(response: Any, params: Params) -> tuple[str, list]: - """Split the submission into (code, file_specs). - - Files listed in the response take precedence; params["files"] is the - fallback. - """ - code, response_files = _unwrap_payload(response) - file_specs = response_files or _coerce_file_specs(params.get("files")) - return code, file_specs - - -def _answer_code(answer: Any) -> str: - """The code string from the answer field, unwrapping a {code, files} - payload the same way the submission is unwrapped.""" - return _unwrap_payload(answer)[0] + teacher = _coerce_file_specs(params.get("answer_files")) + _coerce_file_specs(params.get("files")) + student = _coerce_file_specs(params.get("response_files")) + teacher_names = {spec.get("name") for spec in teacher} + return [spec for spec in student if spec.get("name") not in teacher_names] + teacher def evaluation_function(response: Any, answer: Any, params: Params) -> Result: @@ -391,7 +365,8 @@ 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 - code, file_specs = _resolve_submission(response, params) + code = str(response) + file_specs = _collect_file_specs(params) violations = check_code_safety(code) if violations: @@ -411,10 +386,10 @@ def evaluation_function(response: Any, answer: Any, params: Params) -> Result: if mode == "demo": result = _evaluate_demo(code, result, files_dir) elif mode == "io_test": - ans = _answer_code(answer) if params.get("use_answer_as_expected_output") else "" + ans = str(answer) if params.get("use_answer_as_expected_output") else "" result = _evaluate_io(code, params.get("tests", []), result, answer=ans, files_dir=files_dir) else: - test_code = _answer_code(answer) if params.get("use_answer_as_test_code") else params.get("test_code", "") + test_code = str(answer) if params.get("use_answer_as_test_code") else params.get("test_code", "") result = _evaluate_unit(code, test_code, result, files_dir=files_dir) for warning in file_warnings: diff --git a/evaluation_function/evaluation_test.py b/evaluation_function/evaluation_test.py index 0bdd549..ef82f2c 100755 --- a/evaluation_function/evaluation_test.py +++ b/evaluation_function/evaluation_test.py @@ -367,152 +367,76 @@ def test_import_of_uploaded_module(self, mock_download): 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 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 TestFilesInResponsePayload(unittest.TestCase): - """The LF web client delivers uploads inside the response payload as - {"code": ..., "files": [...]} rather than in params["files"].""" - @patch("evaluation_function.evaluation.download_files") - def test_response_dict_with_code_and_files(self, mock_download): - mock_download.side_effect = _stub_download({"data.csv": "1,2,3"}) - response = { - "code": "print(open('data.csv').read())", - "files": [{"url": "https://example.com/k", "name": "data.csv"}], - } - result = evaluation_function(response, None, {"mode": "demo"}).to_dict() - - self.assertIn("1,2,3", result["feedback"]) - mock_download.assert_called_once() - passed_specs = mock_download.call_args[0][0] - self.assertEqual(passed_specs, [{"url": "https://example.com/k", "name": "data.csv"}]) - - @patch("evaluation_function.evaluation.download_files") - def test_response_dict_file_entries_are_json_strings(self, mock_download): + def test_file_entries_as_json_strings(self, mock_download): mock_download.side_effect = _stub_download({"data.csv": "42"}) - response = { - "code": "print(open('data.csv').read())", - "files": [json.dumps({"url": "https://example.com/k", "name": "data.csv"})], - } - result = evaluation_function(response, None, {"mode": "demo"}).to_dict() + params = {"mode": "demo", "files": [json.dumps({"url": "https://example.com/k", "name": "data.csv"})]} + result = evaluation_function("print(open('data.csv').read())", None, params).to_dict() self.assertIn("42", result["feedback"]) passed_specs = mock_download.call_args[0][0] self.assertEqual(passed_specs, [{"url": "https://example.com/k", "name": "data.csv"}]) @patch("evaluation_function.evaluation.download_files") - def test_response_is_json_string_of_payload(self, mock_download): - mock_download.side_effect = _stub_download({"data.csv": "7"}) - response = json.dumps({ - "code": "print(open('data.csv').read())", - "files": [{"url": "https://example.com/k", "name": "data.csv"}], - }) - result = evaluation_function(response, None, {"mode": "demo"}).to_dict() - - self.assertIn("7", result["feedback"]) - mock_download.assert_called_once() - - @patch("evaluation_function.evaluation.download_files") - def test_unit_test_mode_reads_files_from_response(self, mock_download): - mock_download.side_effect = _stub_download({"data.csv": "x"}) - response = { - "code": "", - "files": [{"url": "https://example.com/k", "name": "data.csv"}], - } - params = { - "mode": "unit_test", - "test_code": "import os\ndef test_present():\n assert os.path.isfile('data.csv')\n", - } - result = evaluation_function(response, None, params).to_dict() + def test_answer_files_param(self, mock_download): + mock_download.side_effect = _stub_download({"data.csv": "teacher"}) + params = {"mode": "demo", "answer_files": [{"url": "https://example.com/t", "name": "data.csv"}]} + result = evaluation_function("print(open('data.csv').read())", None, params).to_dict() - self.assertTrue(result["is_correct"]) - self.assertIn("1/1 tests passed", result["feedback"]) + self.assertIn("teacher", result["feedback"]) @patch("evaluation_function.evaluation.download_files") - def test_plain_string_response_still_uses_params_files(self, mock_download): - mock_download.side_effect = _stub_download({"data.csv": "9"}) - params = {"mode": "demo", "files": [{"url": "https://example.com/k", "name": "data.csv"}]} - result = evaluation_function("print(open('data.csv').read())", None, params).to_dict() + def test_response_files_param(self, mock_download): + mock_download.side_effect = _stub_download({"mine.csv": "student"}) + params = {"mode": "demo", "response_files": [{"url": "https://example.com/s", "name": "mine.csv"}]} + result = evaluation_function("print(open('mine.csv').read())", None, params).to_dict() - self.assertIn("9", result["feedback"]) + self.assertIn("student", result["feedback"]) @patch("evaluation_function.evaluation.download_files") - def test_response_files_take_precedence_over_params_files(self, mock_download): - mock_download.side_effect = _stub_download({"data.csv": "from_response"}) - response = { - "code": "print(open('data.csv').read())", - "files": [{"url": "https://example.com/response", "name": "data.csv"}], + def test_answer_and_response_files_combined(self, mock_download): + mock_download.return_value = [] + params = { + "mode": "demo", + "answer_files": [{"url": "https://example.com/t", "name": "data.csv"}], + "response_files": [ + {"url": "https://example.com/s1", "name": "mine.csv"}, + {"url": "https://example.com/s2", "name": "data.csv"}, + ], } - params = {"mode": "demo", "files": [{"url": "https://example.com/params", "name": "other.csv"}]} - evaluation_function(response, None, params) + evaluation_function("print('hi')", None, params) + mock_download.assert_called_once() passed_specs = mock_download.call_args[0][0] - self.assertEqual(passed_specs, [{"url": "https://example.com/response", "name": "data.csv"}]) + self.assertEqual(passed_specs, [ + {"url": "https://example.com/s1", "name": "mine.csv"}, + {"url": "https://example.com/t", "name": "data.csv"}, + ]) - def test_response_dict_without_files_no_download(self): + def test_no_files_param_no_download_call(self): with patch("evaluation_function.evaluation.download_files") as mock_download: - evaluation_function({"code": "print('hi')"}, None, {"mode": "demo"}) + evaluation_function("print('hi')", None, {"mode": "demo"}) mock_download.assert_not_called() -class TestAnswerFieldPayload(unittest.TestCase): - """With the file-upload widget the answer field is delivered in the same - {"code", "files"} shape as the submission, not as a bare string.""" - - def test_unit_test_code_from_answer_dict(self): - response = {"code": "def square(n):\n return n * n\n"} - answer = {"code": "def test_sq():\n assert square(4) == 16\n", "files": []} - params = {"mode": "unit_test", "use_answer_as_test_code": True} - result = evaluation_function(response, answer, params).to_dict() - - self.assertTrue(result["is_correct"]) - self.assertIn("1/1 tests passed", result["feedback"]) - - def test_unit_test_code_from_answer_json_string(self): - answer = json.dumps({"code": "def test_ok():\n assert True\n", "files": []}) - params = {"mode": "unit_test", "use_answer_as_test_code": True} - result = evaluation_function({"code": ""}, answer, params).to_dict() - - self.assertIn("1/1 tests passed", result["feedback"]) - - def test_io_expected_output_from_answer_dict(self): - response = {"code": "print(6)"} - answer = {"code": "print(2 * 3)", "files": []} - params = {"mode": "io_test", "use_answer_as_expected_output": True, - "tests": [{"input": ""}]} - result = evaluation_function(response, answer, params).to_dict() - - self.assertTrue(result["is_correct"]) +class TestMalformedFileSpec(unittest.TestCase): - def test_plain_string_answer_still_works(self): - params = {"mode": "unit_test", "use_answer_as_test_code": True} - result = evaluation_function("x = 1", "def test_ok():\n assert True\n", params).to_dict() + 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("1/1 tests passed", result["feedback"]) + self.assertIn("hi", result["feedback"]) + self.assertIn("missing", result["feedback"].lower()) class TestUnexpectedExceptionHandling(unittest.TestCase):