# 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.

---

## T-004 — Job-based `POST /download` + `GET /progress/{job_id}` + `GET /download/{job_id}/file`, in-memory job store, yt-dlp `progress_hooks`, frontend polling + progress bar UI

**Verdict:** PASS_WITH_NOTES

### Findings

1. **[minor]** `app.py:296-315` (`progress_hook`) — `job.percent` is only updated inside `if job.total_bytes:`, so when yt-dlp can't report a `total_bytes`/`total_bytes_estimate` (e.g. some DASH/live formats), the progress bar stays at 0% for the whole download even though `downloaded_bytes` is advancing and shown as text. Not required: the plan explicitly allows "downloaded size only if total is unknown" as the fallback UX, and this doesn't affect the common case (verified live: percent tracked correctly when total is known).
   - Required fix: No.
2. **[minor]** `app.py:296-367` — `progress_hooks` only reports raw download percent; ffmpeg post-processing (mp3 extraction, video mux) after the download hits 100% is not reflected, so the bar can sit at 100% for a few extra seconds while the file is finalized before `status` flips to `finished`. Acceptable per plan (`postprocessor_hooks` was offered as one of two options, not mandatory), and verified it does not stall indefinitely or misreport.
   - Required fix: No.
3. **[nit]** `app.py:356-362` — the friendlier `"The download could not be prepared."` message set by `progress_hook` on a hook-level `status == "error"` is generally overwritten by the broader `except Exception as exc: ... job.error_message = str(exc)` handler in `run_job`, since yt-dlp raises after invoking the hook. In practice this means the browser sees yt-dlp's raw exception text (confirmed in testing: `"ERROR: [generic] not-a-video: Unable to download webpage: HTTP Error 404..."`) rather than the friendlier hook message. Same class of note as the T-001 review (raw exception text surfaced to the browser); not a regression introduced by this task and not required to fix here.
   - Required fix: No.

No blocker or major findings.

### Verification

**Steps performed:**
- Re-read `.ai/PLAN.md` (Phase 2 spec for T-004) and diffed it against `app.py` / `README.md`; the three-endpoint split, `JobState`/`_jobs`/`_jobs_lock`, `progress_hooks`-driven updates, `run_in_executor` fire-and-forget scheduling (`_pending_jobs` set to avoid GC warnings), stale-job TTL sweep piggybacked on `POST /download`, and the frontend polling/progress-bar/button-disable/error-handling all match the plan.
- `python3 -m py_compile app.py` — succeeded.
- Built the actual Docker image (`docker build -t yt-dl-review2 .`) and ran it, then drove the real HTTP flow end-to-end against real YouTube URLs:
  - `POST /download` (playlist-context MP3 URL) → `{"job_id": ...}`; polling `GET /progress/{job_id}` every ~1s showed `status: "downloading"` with `percent`/`speed`/`downloaded_bytes`/`total_bytes` advancing correctly (0 → 61% → 100%), then `status: "finished"`.
  - `GET /download/{job_id}/file` after `finished` → 200, correct MP3 bytes (`file` confirms valid MPEG audio), title-based `Content-Disposition` filename (reusing T-003's sanitization) — auto-delivered with no extra click needed on the client side.
  - Job cleanup confirmed: `GET /progress/{job_id}` after the file was served → 404 `"Download job not found."`, confirming `BackgroundTask(cleanup_job, ...)` removed the job and temp dir after delivery.
  - `GET /progress/<unknown>` and `GET /download/<unknown>/file` → both 404 as specified.
  - Fetched `GET /download/{job_id}/file` on a still-`downloading` job → 409 `"The download is not ready yet."`, matching the plan's pre-completion semantics.
  - **Error path**: posted a non-YouTube, non-existent URL (`http://example.com/not-a-video`) → job transitioned to `status: "error"` with a descriptive `error` message, no server crash (`GET /` still returned 200 immediately after), and `docker logs` showed no traceback — matches "server-side failure shows an error in place of the bar with no crash."
  - Confirmed no temp-dir leak on the error path (`docker exec ... ls /tmp` showed the errored job's temp dir already removed); a separate job's temp dir that was intentionally never fetched was still present, consistent with the documented "removed after delivery, or after a timeout for abandoned jobs" behavior (not fetching a finished job's file is expected to leave it until TTL/next sweep).
  - Cleaned up: stopped the test container, removed the `yt-dl-review2`/`yt-dl-review` test images, removed local scratch files.
- Reviewed `README.md` changes: accurately documents the job/progress/file endpoint flow, automatic delivery, and non-persistent in-memory job state.

**Findings from verification:** All acceptance criteria for T-004 hold:
- Progress bar appears on download start and updates with percent/speed/downloaded-total size (verified live with real percentages/speeds).
- File auto-delivers to the browser on completion with the correct sanitized filename (server-side `fetch` + `Content-Disposition` flow verified; same header-parsing logic as T-003, already confirmed to round-trip correctly in the browser).
- Server-side failure shows an error in place of the bar with no crash (verified: job marked `error`, server remained responsive).
- `python -m py_compile app.py` passes.

**Risks:**
- Progress percent can remain at 0% for formats where yt-dlp cannot report a total size (noted above, minor, not required).
- The in-memory job store means an app restart mid-download loses all job state/progress for any in-flight browser sessions; this matches the explicitly stated "no persistent storage" design constraint and is now documented in `README.md`.

---

## T-005 — `Dockerfile` (install Deno JS runtime) + `app.py` (point yt-dlp `ydl_opts` at the installed Deno binary and permit official EJS solver retrieval) + `README.md` update

**Verdict:** PASS

### Findings

No blocker, major, minor, or nit findings.

### Verification

**Steps performed:**
- Re-read `.ai/PLAN.md` (Phase 1 spec for T-005) and diffed it against `Dockerfile`, `app.py`, `README.md`, `ROADMAP.md`. The Dockerfile change (install `curl`/`unzip`, run the official Deno install script into `/usr/local`), the `ydl_opts` additions (`js_runtimes`, `remote_components`), and the README note all match the plan.
- `python3 -m py_compile app.py` — succeeded.
- `docker build -t yt-dl .` — succeeded (cached from the implementer's build, layers unchanged).
- Confirmed `shutil` is already imported at the top of `app.py` (used elsewhere for `rmtree`), so no missing-import risk from the new `shutil.which("deno")` call.
- **Verified the `js_runtimes`/`remote_components` `ydl_opts` shape against the actual installed yt-dlp version inside the built image** (yt-dlp `2026.08.19`), since the plan flagged this as needing confirmation at implementation time:
  - Inspected `yt_dlp/YoutubeDL.py` inside the container: `self.params['js_runtimes']` is validated by `_clean_js_runtimes`, which requires exactly `dict[str, dict|None]` (`{"deno": {"path": ...}}`) — matches `app.py`'s shape precisely, including that `config.get('path')` is what's read internally.
  - Inspected `yt_dlp/options.py`: `dest='js_runtimes'` and `dest='remote_components'` confirm these are the correct `ydl_opts` keys for the installed version.
  - Constructed a `yt_dlp.YoutubeDL(...)` instance inside the container with the exact `ydl_opts` dict `app.py` builds — no warnings/errors, confirming the options are accepted and not silently ignored.
- Ran the container (`docker run --rm -d -p 8081:8080 ...`) and:
  - `docker exec ... deno --version` → `deno 2.9.6` — binary present and runnable, on `PATH`.
  - Live MP3 download of the previously-failing URL (`https://www.youtube.com/watch?v=7MrdyaSlOfI`) via the real job flow (`POST /download` → poll `GET /progress/{job_id}` → `GET /download/{job_id}/file`) — completed successfully, file delivered with the correct sanitized title-based filename via `Content-Disposition`.
  - `docker logs` during that download showed `[youtube] [jsc:deno] Solving JS challenges using deno` and `[youtube] [jsc:deno] Downloading challenge solver lib script from https://github.com/yt-dlp/ejs/releases/...` — confirms Deno and the GitHub-hosted EJS solver are actively used, not just installed-but-unused.
  - Full container log for both live downloads showed **no** `No supported JavaScript runtime could be found` warning anywhere.
  - Regression check: re-ran a playlist-context URL (`watch?v=7MrdyaSlOfI&list=RDMM7MrdyaSlOfI`) through the same job flow — logs show only the single video's extraction/download (no playlist enumeration), progress percent/speed/downloaded/total advanced correctly through completion, and the delivered filename matched the sanitized video title — confirms T-003/T-004 behavior is unaffected.
  - Stopped and removed the test container.
- Reviewed the `README.md` addition: accurately explains the Deno runtime dependency, the GitHub-hosted EJS remote component, why they're needed (JS-based signature/PO-token deciphering), and the GitHub reachability requirement.
- Reviewed the `ROADMAP.md` Priority 3 entry: accurately reflects the bug, root cause, and fix, consistent with what was implemented and verified.

**Findings from verification:** All acceptance criteria for T-005 hold:
- `docker build` completes without errors after adding Deno.
- The previously-failing video URL now downloads successfully end-to-end.
- Server logs no longer show the missing-JS-runtime warning during a normal download.
- Priority 1/2 behavior (single-video-from-playlist, progress bar, filename sanitization) is unaffected.
- `python -m py_compile app.py` passes.

**Risks:**
- The fix depends on outbound network access to `github.com` (for the EJS solver script) and `deno.land` (Dockerfile install-time only); if GitHub is unreachable from the container at runtime, the EJS-dependent extraction path could fail again — this is already called out in `README.md` as a requirement, so no action needed.
- `js_runtimes`/`remote_components` are relatively new, evolving yt-dlp options (confirmed only against the currently pinned `requirements.txt` range, `yt-dlp>=2024.4.9`, as resolved to `2026.08.19` in the built image); a future yt-dlp release changing this option's shape would need to be caught by re-running this same build+live-download validation, not by `py_compile` alone.

---

## T-006 — `app.py` (`extractor_args.youtube.player_client` ordering in shared `ydl_opts`) + `README.md` update

**Verdict:** PASS

### Findings

No blocker, major, minor, or nit findings.

### Verification

**Steps performed:**
- Re-read `.ai/PLAN.md` (Priority 4 / T-006 spec) and diffed it against `app.py`, `README.md`, `ROADMAP.md`. The single-line addition — `"extractor_args": {"youtube": {"player_client": ["android", "ios", "tv", "web"]}}` in the shared `ydl_opts` dict, applied uniformly before the per-mode `format` update — matches the plan exactly. `README.md` documents the client-ordering mitigation and its best-effort nature; `ROADMAP.md` Priority 4 accurately describes the bug, fix, and acceptance criteria.
- `python3 -m py_compile app.py` — succeeded.
- `docker build -t yt-dl .` — succeeded (cached, layers unchanged since T-005).
- Verified the `extractor_args`/`player_client` key is genuinely consumed by the installed yt-dlp (`2026.08.19`) inside the built image, not silently ignored: `grep player_client` in `yt_dlp/extractor/youtube/_video.py` shows `self._configuration_arg('player_client')` is read directly by the extractor, and constructing a real `yt_dlp.YoutubeDL(...)` with the exact `ydl_opts` `app.py` builds raised no warnings/errors.
- Ran the container and drove live downloads through the real job flow (`POST /download` → poll `GET /progress/{job_id}` → `GET /download/{job_id}/file`):
  - **Previously-failing video** (`LV-NXucnyrc`, mode `mp3`) — completed successfully. `docker logs` showed `WARNING: ... HTTP Error 429: Too Many Requests` on the initial webpage fetch (the originally-reported failure trigger), immediately followed by `Downloading android player API JSON`, `Downloading ios player API JSON`, `Downloading tv client config`/`tv player API JSON` — confirming the alternate clients were tried and used. No `Missing required Visitor Data` or `This video is not available` appeared; the job reached `status: "finished"` and the file was delivered via `Content-Disposition` with a correctly sanitized title-based filename (`Baby's 1st Space Adventure ....mp3`), verified as a valid MP3 with `file`.
  - **Regression: plain MP3** (`jNQXAC9IVRw`) — unaffected, unchanged from prior task behavior.
  - **Regression: plain Video** (`jNQXAC9IVRw`, mode `video`) — completed, delivered a valid MP4 (`file` confirms ISO Media MP4) with filename `Me at the zoo.mp4`.
  - **Regression: playlist-link** (`jNQXAC9IVRw&list=RDMMjNQXAC9IVRw`, mode `mp3`) — `docker logs` showed `Downloading just the video jNQXAC9IVRw because of --no-playlist`, confirming T-003's single-video behavior is unaffected by the client-ordering change.
  - **Error path**: posted a non-existent URL (`http://example.com/not-a-video`) — job transitioned to `status: "error"` with a clear `error` message (`HTTP Error 404: Not Found`), no server crash (`GET /` still returned 200 immediately after), confirming the existing error-handling path is unaffected.
  - Progress bar percent/speed/downloaded/total advanced correctly for all live jobs.
  - Stopped and removed the test container, deleted downloaded test files.

**Findings from verification:** All acceptance criteria for T-006 hold:
- A video URL previously failing with `Missing required Visitor Data` / `This video is not available` now downloads successfully via an alternate player client (confirmed live with `LV-NXucnyrc`).
- Server logs show the alternate player clients (`android`/`ios`/`tv`) being used, and no PO-Token-blocking failure occurred.
- Existing playlist-single-video/progress-bar/filename/Deno behavior (T-003/T-004/T-005) is unaffected — all regression checks passed.
- Failures still surface a clean UI error with no crash (verified with the error-path check).
- `python -m py_compile app.py` passes.

**Risks:**
- This is an explicitly best-effort mitigation (as documented in `README.md` and `ROADMAP.md`): YouTube's anti-bot posture changes over time, and the `android`/`ios`/`tv` clients could themselves become restricted later, requiring the client list to be retuned. No action needed now; this is a known, accepted risk per the plan's scope.
- The live test observed `WARNING: ... Some android/ios client https formats have been skipped ... SABR-only streaming experiment` — a yt-dlp/YouTube-side warning unrelated to this change (it did not block the download, format selection fell through to a working format) — noted for awareness, not a regression introduced by T-006.