Revert "Refactor file-handling logic to unify answer_files and response_files handling"

This reverts commit ee80ab9.

Author: m-messer

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["answer_files"]`/`params["response_files"]` objects into the per-request working directory |
+| `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. 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
+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
@@ -93,28 +93,32 @@ All source lives in `evaluation_function/`:
     "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.
+# 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.
 {
     "mode": "demo",
-    "answer_files": [
+    "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`)
@@ -125,7 +129,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["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.
+`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
 
@@ -180,7 +184,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`). Not needed for `params["files"]` / response-payload file 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

@@ -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 may serialise each upload entry to a string.
+    client currently serialises each upload entry to a string.
     """
     if not isinstance(raw, (list, tuple)):
         return []
@@ -343,19 +343,45 @@ def _coerce_file_specs(raw: Any) -> list:
     return specs
 
 
-def _collect_file_specs(params: Params) -> list:
-    """Gather the files to make available for this request.
+def _unwrap_payload(value: Any) -> tuple[str, list]:
+    """Return (code, file_specs) from a submission or answer value.
 
-    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.
+    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.
     """
-    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
+    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]
 
 
 def evaluation_function(response: Any, answer: Any, params: Params) -> Result:
@@ -365,8 +391,7 @@ 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 = str(response)
-    file_specs = _collect_file_specs(params)
+    code, file_specs = _resolve_submission(response, params)
 
     violations = check_code_safety(code)
     if violations:
@@ -386,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: