# Deliberation: technical — Gemini 3 Pro (Preview)

**Date**: 2026-03-17 23:44
**Model**: Gemini 3 Pro (Preview)
**Type**: technical
**Artifact**: ## Cloud Upload v1 — Full Deliverable  ### 1. Server-side: PUT /api/upload (Common Lisp, add to serv

---

Here is the critique of the provided artifact.

### Hallucination Detection

- `trivial-utf-8:string-to-utf-8-bytes`: **[VERIFIED]** A common library in the Lisp ecosystem.
- `myway:*env*`, `myway:connect`: **[VERIFIED]** Matches standard MyWay usage.
- `gethash "content-type" headers`: **[VERIFIED]** Clack environments standardly provide headers as a hash table.
- `:raw-body`: **[VERIFIED]** Clack standard environment key for the input stream.
- `bt:with-lock-held`: **[VERIFIED]** Bordeaux-Threads API.

**Result**: No hallucinations detected.

---

### Architectural & Security Critique

This implementation is currently unsafe for a production or internet-facing environment (even behind Nginx), primarily due to disk exhaustion risks and inadequate input validation.

#### 1. Security: DOS & Disk Exhaustion
**Critique**: The implementation relies solely on the `Content-Length` header for size validation.
- **Problem**: A malicious client can send a `Content-Length` of `100` but stream 50GB of data. The current `loop` reads until the stream ends, which will fill the disk.
- **Fix**: You must enforce the limit on the *actual bytes read* inside the I/O loop. Throw an error and delete the temp file if the byte count exceeds `*max-upload-size*`.

#### 2. Data Integrity: Partial Files
**Critique**: You are writing directly to the destination folder with `:if-exists :supersede`.
- **Problem**: If the upload is interrupted (network failure, server crash), a corrupt/truncated music file remains in the library folder. The subsequent `rescan` will attempt to index a broken file, potentially crashing the scanner or polluting the database.
- **Fix**: Write to a temporary file (e.g., in `/tmp` or a `.staging` subdirectory). Only after the upload stream is fully successful and validated should you atomically `rename-file` to the final destination.

#### 3. Concurrency: Collision Risk
**Critique**: `(get-universal-time)` has 1-second resolution.
- **Problem**: Two uploads occurring within the same second with the same filename will result in a race condition. Since you use `:supersede`, one will silently overwrite the other.
- **Fix**: Use a GUID/UUID or a high-precision timestamp (if available via `local-time`) plus a random suffix for the filename.

#### 4. Security: Input Sanitization
**Critique**: `sanitize-filename` is insufficient.
- **Problem**:
    - It allows control characters (e.g., newlines).
    - It allows filenames starting with `-` (which can confuse CLI tools if those files are ever processed by shell scripts).
    - It does not truncate extremely long filenames (filesystem limits).
- **Fix**: Whitelist allowed characters (alphanumeric, `.`, `-`, `_`). Truncate to 255 chars. Reject null bytes explicitly.

#### 5. Validation: Content-Type
**Critique**: `(member content-type ... :test #'string-equal)` is brittle.
- **Problem**: It fails on `audio/mpeg; charset=binary` or other parameter-appended MIME types.
- **Fix**: Parse the content-type string to extract the base media type before comparison.

#### 6. Performance: Synchronous Rescan
**Critique**: Calling `rescan` inside the request handler while holding `*rescan-lock*`.
- **Problem**: If scanning is slow (calculating durations, hashing, cover art extraction), the HTTP request will hang. While acceptable for a single-user app roughly, it blocks the thread.
- **Fix**: Offload the rescan to a worker queue or a separate thread, or simply return "Upload accepted" and let the scan happen asynchronously. At the very least, acknowledge the potential timeout risk.

#### 7. Code Correctness: HTTP Status Codes
**Critique**: Sending `200` for "Upload not configured".
- **Problem**: This is a server configuration error.
- **Fix**: Return `503 Service Unavailable` or `500 Internal Server Error`.

### Actionable Feedback for the AI Agent

1.  **Implement Byte Counting**: Modify the `loop` to count bytes read. If `current-bytes > *max-upload-size*`, stop reading, close stream, delete parts, and return 413.
2.  **Atomic Write Pattern**:
    - `with-open-file` to a temporary path (e.g., `(uiop:with-temporary-file ...)` or a manual random path).
    - On success: `(rename-file temp-path final-dest)`.
    - On error: `(delete-file temp-path)`.
3.  **Refine Sanitization**:
    - Replace the filter with an allowlist approach.
    - Ensure the filename doesn't start with a dot or dash.
4.  **Fix Content-Type Matching**: Use a utility to strip `; parameter=value` before checking against `+allowed-upload-types+`.
5.  **Robust Path Handling**: When using `merge-pathnames`, ensure `*upload-dir*` is treated as a directory pathname (ends in `/`), otherwise standard CL `merge-pathnames` behavior might merge it into the filename of the parent directory. Use `uiop:ensure-directory-pathname`.

<details>
<summary>Session State</summary>

**Blocking findings**: Safety (Disk exhaustion vulnerability), Irreversibility (Interrupted uploads corrupt library).
**Advisory findings**: Filename sanitization improvements, async processing for rescan, strict content-type parsing.
**Decisions**: Must use atomic rename pattern and enforce streaming read limit.
**Open Questions**: None.
**Key Entities**: `server.lisp`, `sanitize-filename`, `upload-file`.
</details>