mixboard/.orchestra/deliberations/2026-03-17-234428-GPT-5-3-Codex-technical.md

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.

Session State

**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`.