# Deliberation: technical — GPT-5.3-Codex

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

---

### Architectural / Code Pattern Review

#### 1) Common Lisp upload handler (Woo/Clack pattern)

- `:raw-body` stream read loop is structurally correct for raw PUT uploads, **if** `myway:*env*` actually exposes a binary stream there **[UNVERIFIED]**.
- Header access via `(gethash ... headers)` is only correct if `:headers` is a hash table **[UNVERIFIED]**. In many Clack stacks, headers are alist/plist-like.
- Returning `200` for `"Upload not configured"` is wrong. Use `5xx` (`503` preferred) to avoid false client success.
- `content-type` equality check is too strict. Real clients often send `audio/mpeg; charset=binary`. Parse media type before matching.
- Size enforcement is incomplete:
  - If `Content-Length` missing/invalid, limit is bypassed.
  - No streaming byte counter; attacker can exhaust disk.
  - `parse-integer :junk-allowed t` is unsafe (`"123abc"` parses as `123`).
- Mid-stream interruption edge case is not handled safely:
  - Partial file may remain on disk if read/write errors occur.
  - Use temp file + atomic rename only on success; delete temp on error/cancel.
- Filename collision claim is incorrect: second-level timestamp can collide under concurrency. `:if-exists :supersede` can overwrite. Use UUID/random suffix.
- `sanitize-filename` is insufficient:
  - Does not remove control chars, `:` (macOS path concerns), or enforce max length.
  - Should normalize to strict allowlist, e.g. `[A-Za-z0-9._-]`, truncate, and reject empty.
- `bt:with-lock-held (*rescan-lock*)` around full `rescan + save-db` can become a throughput bottleneck; consider narrower critical section or async indexing queue.

#### 2) Swift concurrency / Sendable

- `@MainActor final class ... @unchecked Sendable` is a red flag.  
  `@unchecked Sendable` is likely unnecessary and weakens guarantees. Remove unless compiler requires it for a proven-safe cross-actor path.
- `URLSessionTaskDelegate` callback is `nonisolated`, which is correct, and hopping via `Task { @MainActor ... }` is safe.
- Cancellation behavior is ambiguous **[CHECK THIS]**:
  - You cancel wrapper `Task`, but no explicit handle to underlying `URLSessionTask`.
  - Usually propagated, but if you need deterministic cancel semantics, use `uploadTask(with:fromFile:)` and store/cancel the task directly.
- Progress callback can race with `cancel()`/`dismiss()` and repopulate progress after reset. Gate updates with an upload token/ID.
- URL construction by string concatenation is brittle. Use `URLComponents` or `appendingPathComponent("api/upload")`.
- `X-Filename` should be sanitized client-side too (strip CR/LF, control chars) before setting header.

### Data model / API usage

- Error model is inconsistent: server returns `+400+` for unsupported type (likely generic) **[UNVERIFIED]**; client expects meaningful `message`. Return explicit JSON error for each reject path.
- Server does not validate body presence; zero-byte upload can pass and trigger rescan.
- Consider idempotency: repeated upload of same file currently creates duplicates unless rescan dedups **[UNVERIFIED]**.

### Security findings

- High risk: unbounded upload without strict streaming cap.
- Medium risk: insufficient filename sanitization (control chars, long names, hidden files).
- Medium risk: possible header/value trust issues with `X-Filename`.
- Auth relies on `with-user` **[UNVERIFIED]**; ensure endpoint cannot be hit anonymously.

### Hallucination Detection on artifact claims

- “Raw body PUT matches existing server patterns” → **[UNVERIFIED]**
- “Task-based cancellation” fully cancels network transfer → **[CHECK THIS]**
- “`bt:with-lock-held` avoids racing with background rescanner” → **[UNVERIFIED]** (depends on all codepaths using same lock)
- “unix-timestamp-sanitized-filename prevents collisions” → **[CHECK THIS]** (does not under same-second collisions)
- “strips `/` and `\` (no path traversal)” → partially true, but sanitization remains incomplete; not sufficient as a full security claim.

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

**Blocking findings**:  
- No enforced streaming max size when `Content-Length` is missing/invalid (disk exhaustion risk).  
- Partial-file handling is unsafe (no temp file + cleanup on interrupted upload).  
- Incorrect success status (`200`) for server misconfiguration.  
- Filename collision risk due second-based naming + `:supersede`.

**Advisory findings**:  
- Parse `Content-Type` robustly (handle parameters).  
- Strengthen filename sanitization (allowlist, truncate, control-char strip).  
- Remove/justify `@unchecked Sendable`.  
- Use deterministic upload task cancellation and progress tokening.  
- Replace URL string concatenation with path-safe URL APIs.  
- Revisit coarse rescan lock scope for performance.

**Decisions**:  
- Recommend temp-file write + byte-count enforcement + atomic rename as required server pattern.  
- Recommend explicit structured error responses for each reject path.

**Open Questions**:  
- Is `:headers` truly a hash table in this MyWay/Clack setup?  
- Does `with-user` guarantee auth on this route?  
- Does background rescanner always honor `*rescan-lock*`?  
- Does `rescan` deduplicate by content/hash or path only?

**Key Entities**:  
`upload-file`, `sanitize-filename`, `*max-upload-size*`, `+allowed-upload-types+`, `myway:*env*` **[UNVERIFIED]**, `:raw-body`, `*rescan-lock*` **[UNVERIFIED]**, `rescan`, `save-db`, `UploadService`, `URLSessionTaskDelegate`, `performUpload`, `X-Filename`.
</details>