Merge pull request #1 from lambda-feedback/feature/file_upload

Feature/file upload

Author: m-messer

CLAUDE.md

@@ -11,17 +11,19 @@ All source lives in `evaluation_function/`:
 | `main.py` | IPC server entry point; registers `evaluation_function` and `preview_function` with lf_toolkit |
 | `evaluation.py` | Core evaluation pipeline: security check → subprocess execution → output comparison → plot upload (GCS/S3 via lf_toolkit) → structured feedback |
 | `preview.py` | AST-based pre-execution security validator (`_SecurityVisitor`) |
+| `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. Dispatch by `params["mode"]` (required):
+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
    - **`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 via `lf_toolkit` `upload_image` (`_UPLOAD_FOLDER = "evaluatePython"`); backend is GCS or S3 per `IMAGE_UPLOAD_BACKEND`
-4. Return a `Result` with feedback tags: `pass`, `fail`, `hidden_fail`, `error`, `output`, `summary`
+4. Upload any captured matplotlib figures via `lf_toolkit` `upload_image` (`_UPLOAD_FOLDER = "evaluatePython"`); backend is GCS or S3 per `IMAGE_UPLOAD_BACKEND`
+5. Return a `Result` with feedback tags: `pass`, `fail`, `hidden_fail`, `error`, `output`, `summary`
 
 ### Request shape
 
@@ -90,16 +92,41 @@ All source lives in `evaluation_function/`:
     "pep8_feedback": ["E225", "E231"],  # custom rule list
     "tests": [...]
 }
+
+# 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",
+    "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"},
+    ]
+}
 ```
 
 ### 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["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
 
 ```bash
@@ -153,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`) |
+| `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 |

evaluation_function/evaluation.py

@@ -3,14 +3,17 @@
 import os
 import shutil
 import subprocess
+import sys
 import tempfile
+import traceback
 from typing import Any
 
 import pycodestyle
 from PIL import Image
 from lf_toolkit.evaluation import Result, Params
 from lf_toolkit.evaluation.image_upload import upload_image, ImageUploadError
 
+from .s3_files import download_files
 from .security import check_code_safety
 
 _TIMEOUT = 25
@@ -32,10 +35,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:
@@ -109,19 +129,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 = []
@@ -135,8 +158,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:
@@ -169,9 +194,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:
@@ -183,7 +208,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)
 
@@ -206,12 +231,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}"
 
@@ -248,15 +273,15 @@ def _evaluate_io(response: str, tests: list, result: Result, answer: str = "") -
     return result
 
 
-def _evaluate_unit(response: str, test_code: str, result: Result) -> Result:
+def _evaluate_unit(response: str, test_code: str, result: Result, files_dir: str | None = None) -> Result:
     if not test_code.strip():
         result.add_feedback("error", "No test code provided for unit_test mode.")
         return result
 
     results_path = tempfile.mktemp(suffix=".json")
     runner = _UNIT_RUNNER_TEMPLATE.format(results_path=results_path)
     combined = _add_repl_print(response) + "\n\n" + test_code + runner
-    stdout, stderr, timed_out, _ = _run_code(combined, "")
+    stdout, stderr, timed_out, _ = _run_code(combined, "", files_dir)
 
     test_results = None
     try:
@@ -298,38 +323,97 @@ def _evaluate_unit(response: str, test_code: str, result: Result) -> Result:
     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 may serialise 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 _collect_file_specs(params: Params) -> list:
+    """Gather the files to make available for this request.
+
+    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.
+    """
+    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:
     result = Result()
     mode = params.get("mode")
     if mode not in ("demo", "io_test", "unit_test"):
         result.add_feedback("error", f"Unknown or missing mode: {mode!r}. Expected 'demo', 'io_test', or 'unit_test'.")
         return result
 
-    violations = check_code_safety(str(response))
+    code = str(response)
+    file_specs = _collect_file_specs(params)
+
+    violations = check_code_safety(code)
     if violations:
         result.add_feedback(
             "error",
             "Unsafe code detected -- not executed:\n" + "\n".join(f"- {v}" for v in violations),
         )
         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)
-
-    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)
+    files_dir = None
+    try:
+        file_warnings: list[str] = []
+        if file_specs:
+            files_dir = tempfile.mkdtemp()
+            file_warnings = download_files(file_specs, files_dir)
+
+        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 ""
+            result = _evaluate_io(code, params.get("tests", []), result, answer=ans, files_dir=files_dir)
         else:
-            body = "No style issues found."
-        result.add_feedback("style", body)
+            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:
+            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(code, 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)
 
     return result