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.

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["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 |

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: