youtubedownloader/.ai/REVIEW.md

REVIEW

Append-only review log. Each task section is appended or updated in place; prior task history is preserved.

---

T-001 — FastAPI backend (app.py) + inline HTML/JS frontend (requirements.txt)

Verdict: PASS_WITH_NOTES

Findings

1. [minor] app.py:147 — On any failure inside download() (including yt-dlp errors), str(exc) is returned verbatim to the browser as the message field. yt-dlp exceptions can include local filesystem paths (the temp dir) or verbose internal detail. Not a required fix for this local/self-hosted tool, but consider mapping to a generic message (e.g. "The video could not be downloaded.") and logging the raw exception server-side instead, before this is exposed beyond local/dev use.
   • Required fix: No.

No blocker or major findings.

Verification

Steps performed:

• Re-read .ai/PLAN.md (Phase 1 spec for T-001) and diffed it against app.py / requirements.txt.
• python -m py_compile app.py — succeeded.
• Installed fastapi, uvicorn, httpx, python-multipart, yt-dlp into a scratch venv and exercised the app with starlette.testclient.TestClient, mocking yt_dlp.YoutubeDL to avoid live network calls:
  • GET / → 200, body contains "Download MP3" and "Download Video".
  • POST /download with a garbage (non-http) URL → 400 JSON {"message": ...}.
  • POST /download with an empty URL → 400 JSON.
  • POST /download missing the mode field (triggers RequestValidationError handler) → 400 JSON with .message.
  • POST /download with a valid URL, mode mp3, mocked yt-dlp writing a fake file → 200, correct bytes streamed, Content-Disposition: attachment; filename="...", temp dir cleaned up via BackgroundTask.
  • POST /download where mocked yt-dlp raises RuntimeError → 500 JSON {"message": "..."} (no server crash), temp dir cleaned up on the error path.
  • Filename containing non-ASCII characters (e.g. "café mix 🎵.mp3") → correctly RFC 5987-encoded as filename*=utf-8''... by Starlette; no crash or mojibake.

Findings from verification: All acceptance criteria for T-001 hold:

• GET / returns 200 with HTML containing "Download MP3".
• POST /download with a valid URL + mode streams a file.
• Invalid URL returns 400 JSON with .message.

Risks:

• No live network test against real YouTube was performed (would require external network access and a real video URL); mocked yt-dlp calls confirm the FastAPI plumbing (routing, form validation, error handling, file streaming, temp-dir cleanup) is correct, but do not confirm yt-dlp/ffmpeg behavior against a live video. This risk carries into the Docker-level validation planned for T-002 (docker build + smoke test), where a live download should be attempted at least once.
• Large/long-running downloads have no timeout or size cap — acceptable for v1 per ROADMAP.md (out of scope), but worth revisiting if this moves beyond local/dev use.

---

T-002 — Dockerfile + README.md (build/run docs)

Verdict: PASS

Findings

1. [nit] Dockerfile — the container runs as root (no USER directive). Not required for a local/self-hosted v1 tool per ROADMAP.md, but worth adding a non-root user if this is ever exposed beyond a trusted local network.
   • Required fix: No.

No blocker, major, or minor findings.

Verification

Steps performed:

• Re-read .ai/PLAN.md (Phase 2 spec for T-002) and diffed it against Dockerfile / README.md; matches the planned Dockerfile contents and documented build/run/usage/size-note requirements.
• docker build -t yt-dl-review . — exited 0.
• docker run --rm -d -p 8091:8080 yt-dl-review — container started; curl http://localhost:8091/ returned HTTP 200 with both "Download MP3" and "Download Video" present in the body.
• POST /download with an invalid URL against the running container → 400 JSON {"message": "..."}", confirming the containerized app behaves the same as the local dev checks from T-001; docker logs showed clean request handling with no crash/traceback.
• Live end-to-end test (real network, real YouTube video https://www.youtube.com/watch?v=jNQXAC9IVRw, "Me at the zoo"):
  • mode=mp3 → 200, content-type: audio/mpeg, valid ID3-tagged MP3 file returned (verified with file), Content-Disposition filename correctly derived from video title.
  • mode=video → 200, content-type: video/mp4, valid ISO-Media MP4 file returned (verified with file).
  • This closes the live-download risk flagged in the T-001 review — yt-dlp + ffmpeg inside the container work correctly end-to-end for both modes.
• Cleaned up: stopped test container, removed the yt-dl-review test image, removed downloaded test files.

Findings from verification: All acceptance criteria for T-002 hold:

• docker build -t yt-dl . exits 0.
• docker run --rm -p 8080:8080 yt-dl starts the server.
• curl http://localhost:8080/ returns HTML with both download buttons.
• (Bonus, beyond stated AC) A real MP3 and a real video download both succeed end-to-end through the container.

Risks:

• None outstanding. The live-download risk noted in the T-001 review has been verified and closed here.

---

T-003 — noplaylist option, sanitize_filename helper, title-based filenames, frontend Content-Disposition fix

Verdict: PASS

Findings

No blocker, major, minor, or nit findings.

Verification

Steps performed:

• Re-read .ai/PLAN.md (Phase 1 spec for T-003) and diffed it against app.py / README.md; implementation matches the plan (noplaylist: True on both modes, sanitize_filename helper with the specified regex, extract_info(..., download=True) used to recover title/id, filename=display_name passed to FileResponse, frontend Content-Disposition-based link.download fix).
• python3 -m py_compile app.py — succeeded.
• Unit-checked sanitize_filename directly (scratch venv, no mocks):
  • sanitize_filename("Song 🔥 Title ✨ (Live)", "abc123") → "Song Title (Live)".
  • sanitize_filename("🔥🔥🔥", "abc123") → "abc123" (fallback to id).
  • sanitize_filename("日本語 タイトル", "abc123") → "日本語 タイトル" (non-Latin preserved).
  • Verified Starlette's FileResponse encodes such filenames via filename*=utf-8''... (RFC 5987) when they contain non-ASCII/space/apostrophe characters, and confirmed the frontend regex (/filename\*?=(?:UTF-8''|")?([^";]+)/i) plus decodeURIComponent correctly recovers the original title from that header format.
• Live end-to-end test, built and ran the actual Docker image (docker build -t yt-dl-review ., docker run -p 8081:8080):
  • GET / → 200, contains "Download MP3".
  • POST /download with a playlist-context URL (watch?v=dQw4w9WgXcQ&list=RDdQw4w9WgXcQ), mode=mp3 → 200; container logs show Downloading just the video dQw4w9WgXcQ because of --no-playlist, confirming only the referenced video is fetched, never the rest of the playlist/mix; response is a single valid MP3 (file confirms ID3 v2.4 MPEG audio) with Content-Disposition: attachment; filename*=utf-8''Rick Astley - Never Gonna Give You Up (Official Video) (4K Remaster).mp3 — title-based, not a UUID.
  • mode=video against a plain (non-playlist) URL → 200, valid MP4 (file confirms ISO Media MP4), filename Me at the zoo.mp4 derived from the video title.
  • POST /download with an invalid (non-http) URL → unchanged 400 JSON {"message": ...} behavior, confirming the existing validation path was not regressed.
  • Cleaned up: stopped/removed the test container and scratch venv, deleted downloaded test files.
• Reviewed README.md changes; the new paragraph accurately describes the playlist-single-video behavior and the title-based/emoji-stripped filename rule, matching observed behavior.

Findings from verification: All acceptance criteria for T-003 hold:

• Pasting a playlist/list= URL downloads only the referenced video (confirmed via yt-dlp's own --no-playlist log line and a single file being produced).
• Saved filename matches the video title with emoji/icons stripped (normal chars, including non-Latin, kept) instead of a UUID, with the correct extension.
• python -m py_compile app.py passes.

Risks:

• None outstanding. sanitize_filename strips characters outside \w/whitespace/- _ . ( ) , '; this only affects the Content-Disposition header value used for the browser's save-as name, not the on-disk path used to read the file, so there is no path-traversal or injection concern from unsanitized titles.