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

This reverts commit 580083e.

Author: m-messer

CLAUDE.md

@@ -11,19 +11,17 @@ 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. 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):
+2. 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
-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`
+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`
 
 ### Request shape
 
@@ -92,41 +90,16 @@ 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 and blocks:
+`_SecurityVisitor` walks the AST before any execution and blocks:
 
-- **Modules**: `os`, `sys`, `subprocess`, `socket`, `urllib`, `http`, `requests`, `shutil`, `ftplib`, `smtplib`, `ctypes`, `multiprocessing`, `threading`, `importlib`, `pickle`, `builtins`
-- **Builtins**: `exec`, `eval`, `compile`, `__import__`
+- **Modules**: `os`, `sys`, `subprocess`, `socket`, `urllib`, `http`, `requests`, `shutil`, `pathlib`, `ftplib`, `smtplib`, `ctypes`, `multiprocessing`, `threading`, `importlib`, `pickle`, `builtins`
+- **Builtins**: `exec`, `eval`, `compile`, `open`, `__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
@@ -180,7 +153,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 `answer_files` / `response_files` 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`) |
 | `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,17 +3,14 @@
 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
@@ -35,27 +32,10 @@ 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:
@@ -129,22 +109,19 @@ def _add_repl_print(code: str) -> str:
     return code + f"\nprint(repr({ast.unparse(node)}))"
 
 
-def _run_code(code: str, stdin: str, files_dir: str | None = None) -> tuple[str, str, bool, list[Image.Image]]:
+def _run_code(code: str, stdin: str) -> tuple[str, str, bool, list[Image.Image]]:
     plot_dir = tempfile.mkdtemp()
-    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:
+    preamble = _PREAMBLE_TEMPLATE.format(plot_dir=plot_dir)
+    with tempfile.NamedTemporaryFile(mode="w", suffix=".py", delete=False) as f:
         f.write(preamble + "\n" + code + "\n" + _CAPTURE_CALL)
+        tmpfile = f.name
     try:
         proc = subprocess.run(
-            ["python", "_submission.py"],
+            ["python", tmpfile],
             input=stdin,
             capture_output=True,
             text=True,
             timeout=_TIMEOUT,
-            cwd=run_dir,
             env={**os.environ, "MPLBACKEND": "Agg", "MPLCONFIGDIR": "/tmp"},
         )
         images = []
@@ -158,10 +135,8 @@ def _run_code(code: str, stdin: str, files_dir: str | None = None) -> tuple[str,
     except subprocess.TimeoutExpired:
         return "", "", True, []
     finally:
-        os.unlink(script_path)
+        os.unlink(tmpfile)
         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:
@@ -194,9 +169,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, files_dir: str | None = None) -> Result:
+def _evaluate_demo(response: str, result: Result) -> Result:
     response = _add_repl_print(response)
-    stdout, stderr, timed_out, images = _run_code(response, "", files_dir)
+    stdout, stderr, timed_out, images = _run_code(response, "")
     if timed_out:
         result.add_feedback("error", f"Code timed out after {_TIMEOUT}s.")
     elif stderr and not stdout:
@@ -208,7 +183,7 @@ def _evaluate_demo(response: str, result: Result, files_dir: str | None = None)
     return result
 
 
-def _evaluate_io(response: str, tests: list, result: Result, answer: str = "", files_dir: str | None = None) -> Result:
+def _evaluate_io(response: str, tests: list, result: Result, answer: str = "") -> Result:
     passed = 0
     response = _add_repl_print(response)
 
@@ -231,12 +206,12 @@ def _evaluate_io(response: str, tests: list, result: Result, answer: str = "", f
         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, files_dir)
+            ans_stdout, _, _, _ = _run_code(ans_run_code, run_stdin)
             expected = ans_stdout.rstrip()
         else:
             expected = test.get("expected_output", "").rstrip()
 
-        stdout, stderr, timed_out, images = _run_code(run_code, run_stdin, files_dir)
+        stdout, stderr, timed_out, images = _run_code(run_code, run_stdin)
         actual = stdout.rstrip()
         label = f"Hidden test {i}" if hidden else f"Test {i}"
 
@@ -273,15 +248,15 @@ def _evaluate_io(response: str, tests: list, result: Result, answer: str = "", f
     return result
 
 
-def _evaluate_unit(response: str, test_code: str, result: Result, files_dir: str | None = None) -> Result:
+def _evaluate_unit(response: str, test_code: str, result: Result) -> 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, "", files_dir)
+    stdout, stderr, timed_out, _ = _run_code(combined, "")
 
     test_results = None
     try:
@@ -323,97 +298,38 @@ 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 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
 
-    code = str(response)
-    file_specs = _collect_file_specs(params)
-
-    violations = check_code_safety(code)
+    violations = check_code_safety(str(response))
     if violations:
         result.add_feedback(
             "error",
             "Unsafe code detected -- not executed:\n" + "\n".join(f"- {v}" for v in violations),
         )
         return result
 
-    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)
+    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)
         else:
-            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)
+            body = "No style issues found."
+        result.add_feedback("style", body)
 
     return result