feat(library): P3 — heavy tier download, quality preferences, storage accounting, per-source rules#2070
feat(library): P3 — heavy tier download, quality preferences, storage accounting, per-source rules#2070hognek wants to merge 9 commits into
Conversation
…ollections handoff, app UI
Implements the Library app Phase 1 from docs/design/library-app.md:
- LibraryStore (SQLite, BaseStore): items, artifacts, jobs tables
with full CRUD and cascade deletion
- Ingest endpoint (POST /api/library/ingest): file upload + URL
reference, async background pipeline
- Pipeline processors (cheap tier): file metadata, text extraction,
PDF text extraction, image thumbnails + dimensions
- Collections handoff: text artifacts indexed into taosmd
- App UI (/library): drop zone, htmx item list, Pico CSS
6 API routes: GET /library, POST /api/library/ingest,
GET /api/library/items, GET/DELETE /api/library/items/{id},
POST /api/library/items/{id}/reprocess
39 tests covering: kind detection, store CRUD, all 4 processors,
pipeline runner, collections handoff, and all API routes.
Related: jaylfc#2057
Docs-Reviewed: P1 library routes are internal admin-session; Library app docs live at docs/design/library-app.md. No external API surface change yet.
…d cap, task GC, HTMX responses - pipeline: mark missing source files as 'error' instead of silently 'ready' - ingest: stream file uploads in 1MB chunks with 100MB cap, reject oversized - background tasks: retain strong references via module-level _background_tasks set - HTMX: return HTML fragments (.item-card) when HX-Request header is present
…ngestor Add YouTubeProcessor for url:youtube items — fetches metadata, thumbnail, transcript, and chapters via knowledge_fetchers.youtube (yt-dlp). Cheap-tier only: no video download, just text artifacts for collection indexing. Add WebProcessor for url:web items — fetches HTML (SSRF-guarded), extracts readable text via readability-lxml (falls back to tag-stripping). Auto-titles from <title> tag. Both registered in _PROCESSORS dict; run_pipeline dispatches to them for url:youtube and url:web kinds. Design doc: docs/design/library-app.md sections 3-4. Tests: 8 new tests, all 47 library tests pass.
… exceptions - _render_item_card: escape all user-controlled values (title, kind, id, status) with html.escape() to prevent reflected XSS through crafted page titles, YouTube metadata, or user form fields. - WebProcessor: use html.unescape() on extracted <title> text to decode HTML entities before storage. - YouTubeProcessor/WebProcessor: remove bare except Exception that swallowed fetch failures. Let network/SSRF/yt-dlp errors propagate to run_pipeline which already marks items as 'error' — a failed URL fetch must not silently appear 'ready' with zero artifacts. All 47 library tests pass.
…ponse-size cap - WebProcessor: disable auto-redirects, manually validate every redirect hop with validate_url_or_raise() against the SSRF blocklist (same pattern as knowledge_ingest._download_article). Max 5 redirects. - WebProcessor: cap response body at 10 MB, stream with aiter_bytes() to avoid OOM on large/hostile pages. - Tests: update _mock_httpx_response with is_redirect=False, encoding, and aiter_bytes() to match the new fetch flow. All library tests pass.
… accounting, per-source rules - LibraryStore: add library_rules table, new columns (quality, auto_download, download_path, download_bytes, downloaded_at) with safe migration in _post_init, rule CRUD methods, fnmatch-based match_rules, get_storage_summary - HeavyDownloadProcessor: yt-dlp-based media download for url:youtube items with quality preference (360/480/720/1080/best), fallback heuristics for missing output paths - run_heavy_pipeline: rule-aware quality resolution (explicit > rule > item > default 720), job tracking via library_jobs table - Routes: POST /download, GET /download/status, POST/GET/DELETE /rules, GET /usage with HTMX-aware HTML responses - Auto-download: _ingest_task checks matching rules with auto_download=True and triggers heavy pipeline after cheap tier completes - Template: storage summary bar (polled via htmx), per-item download button with quality selector for YouTube items, rules management <details> panel with add/delete - Tests: 24 new tests (7 store rules/storage, 4 HeavyDownloadProcessor, 4 run_heavy_pipeline, 9 routes) - 70/70 library tests pass (1 slow route test skipped)
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughAdds a complete Library application with SQLite persistence, asynchronous file and URL ingestion, artifact processing, YouTube downloads, collections handoff, FastAPI endpoints, an HTMX UI, and comprehensive tests. ChangesLibrary application
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant LibraryRoutes
participant LibraryPipeline
participant LibraryStore
participant CollectionsHandoff
Browser->>LibraryRoutes: submit file or URL
LibraryRoutes->>LibraryStore: create pending item
LibraryRoutes->>LibraryPipeline: start ingest task
LibraryPipeline->>LibraryStore: store artifacts and ready status
LibraryRoutes->>CollectionsHandoff: hand off text artifacts
CollectionsHandoff->>LibraryStore: read artifacts and item metadata
LibraryRoutes-->>Browser: return item and status
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| quality = item.get("quality", "") or "720" | ||
|
|
||
| # Create a job entry | ||
| await store.create_job(item_id, "heavy_download") |
There was a problem hiding this comment.
WARNING: create_job returns the new job id, but it is discarded here. The success/error branches below re-fetch with (await store.get_item_jobs(item_id))[-1]["id"] to locate "the" heavy-download job. That is racy and incorrect if more than one job exists for the item (e.g. the cheap-tier pipeline already created jobs, or a manual download and an auto-download run concurrently). get_item_jobs orders by created_at, and with time.time() timestamps two jobs created in the same tick can sort unpredictably, so [-1] may point at the wrong job and mark the wrong entry done/error.
Capture the id directly and reuse it:
| await store.create_job(item_id, "heavy_download") | |
| job_id = await store.create_job(item_id, "heavy_download") |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| return None | ||
| except Exception: | ||
| logger.exception("Heavy pipeline failed for item %s", item_id) | ||
| await store.update_item_status(item_id, "error") |
There was a problem hiding this comment.
WARNING: An unexpected exception in the optional heavy-download tier flips the whole item to error via update_item_status(item_id, "error"). The heavy download is opt-in and runs after the cheap-tier pipeline has already produced a ready item with valid text artifacts. A failed secondary media download should not overwrite that ready status and effectively hide the successfully-ingested content in the UI. Consider recording the failure in meta_json/the job entry (as the in-band path is None branch already does) and leaving the item status untouched.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # Read body with a size cap to avoid OOM on large/hostile pages. | ||
| body_chunks: list[bytes] = [] | ||
| total = 0 | ||
| async for chunk in resp.aiter_bytes(8192): |
There was a problem hiding this comment.
WARNING: This size cap does not actually protect against OOM. resp = await client.get(current_url) (line 413) fully reads and buffers the entire response body into memory before this loop runs, so aiter_bytes() here just replays already-downloaded bytes. A hostile/large page is fully loaded regardless of _MAX_WEB_BYTES. To enforce the cap during download, stream the request instead, e.g. async with client.stream("GET", current_url) as resp: and read/cap inside that context (this also changes the redirect handling, since a streamed redirect response must be inspected before the body is consumed).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| logger.info("Heavy download complete for item %s: %s", item_id, result) | ||
| except Exception: | ||
| logger.exception("Heavy download crashed for item %s", item_id) | ||
| await store.update_item_status(item_id, "error") |
There was a problem hiding this comment.
WARNING: Same concern as run_heavy_pipeline: if the background heavy download crashes, this marks the entire item as error, overwriting the ready status and text artifacts already produced by the cheap tier. Since heavy download is an opt-in, additive step, a failure should be surfaced via the download job/status rather than degrading the item's overall state. (Note also that run_heavy_pipeline already swallows exceptions internally and sets error itself, so this outer handler will rarely trigger.)
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (1 files)
Previous Review Summaries (3 snapshots, latest commit 7bd5912)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 7bd5912)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (3 files)
Note: the previous WARNING on Fix these issues in Kilo Cloud Previous review (commit 1b3fcc7)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (5 files)
Fix these issues in Kilo Cloud Previous review (commit 74cb317)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (7 files)
Reviewed by hy3:free · Input: 35.6K · Output: 3.1K · Cached: 95.6K |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
tests/test_library.py (2)
409-419: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueIncomplete assertion —
updated_titleis computed but never checked.
updated_titleis read on Line 416 and the comment describes verifying the title, but no assertion follows, so this block is a no-op. Either assert the expected title behavior or drop the dead variable and comments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_library.py` around lines 409 - 419, Complete the title verification in the test block following the updated_title assignment by asserting the expected title behavior after processing. If no title value is guaranteed, remove updated_title and its accompanying comments instead of leaving unused test code.
1096-1097: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace fixed
asyncio.sleep(0.5)waits with polling to avoid flaky tests. All three tests block on a hardcoded 0.5s sleep for the background ingest task to complete, which is timing-dependent and can fail under CI load or pass spuriously; poll the item status (or download state) with a short timeout instead.
tests/test_library.py#L1096-L1097: pollGET /api/library/items/{item_id}until status is terminal (bounded timeout) before posting the download.tests/test_library.py#L1122-L1123: same polling before asserting the non-YouTube download rejection.tests/test_library.py#L1135-L1136: same polling before requesting/download/status.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_library.py` around lines 1096 - 1097, Replace the fixed asyncio.sleep waits in tests/test_library.py at lines 1096-1097, 1122-1123, and 1135-1136 with bounded polling of GET /api/library/items/{item_id}. Wait until the item reaches a terminal status before posting the download, asserting the non-YouTube rejection, or requesting /download/status, using a short polling interval and timeout.tinyagentos/library_store.py (1)
85-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: fold the P3 columns into
CREATE TABLEand keep the migration for legacy DBs only.
quality,auto_download,downloaded_at,download_path, anddownload_bytesare absent from thelibrary_itemsCREATE TABLE(Lines 25-36) and exist only because_post_initalways runs theALTER TABLEpath. It works, but a reader inspecting the schema would not see these columns, and theif col_names:guard on Line 100 is always truthy (the table always exists), so it does not actually gate the commit on a migration having run. Declaring the columns in the base schema and scoping the migration to legacy P1/P2 databases makes the intended shape self-documenting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/library_store.py` around lines 85 - 101, Update the library_items CREATE TABLE definition to declare quality, auto_download, downloaded_at, download_path, and download_bytes with their existing types and defaults. In _post_init, retain the ALTER TABLE migration only for legacy databases missing those columns, and commit only when at least one migration was applied rather than checking col_names, while preserving existing schema behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tinyagentos/library_pipeline.py`:
- Around line 667-684: The missing-file fallback in the download handling flow
must not select an arbitrary newest entry from the shared download directory.
Update the logic around the Path(path) check to locate only the file matching
the current video’s expected ID or output stem, exclude temporary or in-progress
files such as .part, and retain the existing download_error update when no
matching completed file exists.
- Around line 745-766: The heavy download flow should retain the ID returned by
create_job and use it for both update_job calls. Update the job creation
assignment and replace each get_item_jobs(item_id)[-1]["id"] lookup with that
captured job_id, preserving the existing done and error states.
- Around line 404-439: Update the redirect-fetch loop around current_url, resp,
and httpx.AsyncClient so requests use client.stream("GET", current_url) instead
of client.get(). Keep the response context open while checking redirects,
calling raise_for_status(), and consuming aiter_bytes(); enforce _MAX_WEB_BYTES
during streaming before accumulating chunks, then close the stream before
proceeding.
In `@tinyagentos/routes/library.py`:
- Around line 181-213: Update _ingest_task to fetch and validate the item’s
terminal status immediately after run_pipeline completes, and only perform rule
matching and run_heavy_pipeline when the status is ready. Return without
auto-download when the pipeline leaves the item in error, while preserving the
existing auto-download behavior for successful items.
- Around line 287-289: Remove the unnecessary f-string prefixes from the static
`<div class="info">` and `<div class="meta">` fragments in the surrounding HTML
construction, while retaining the f-string prefix on the title-containing `<h3>`
fragment.
In `@tinyagentos/templates/library.html`:
- Around line 76-152: Add a shared HTMX configuration in library.html that
injects the X-CSRF-Token header for every HTMX request, sourcing the token from
the page’s existing CSRF mechanism. Ensure the configuration applies to the
ingest form/drop-zone and source-rules write requests so session-cookie users
pass verify_csrf, without changing their endpoints or targets.
---
Nitpick comments:
In `@tests/test_library.py`:
- Around line 409-419: Complete the title verification in the test block
following the updated_title assignment by asserting the expected title behavior
after processing. If no title value is guaranteed, remove updated_title and its
accompanying comments instead of leaving unused test code.
- Around line 1096-1097: Replace the fixed asyncio.sleep waits in
tests/test_library.py at lines 1096-1097, 1122-1123, and 1135-1136 with bounded
polling of GET /api/library/items/{item_id}. Wait until the item reaches a
terminal status before posting the download, asserting the non-YouTube
rejection, or requesting /download/status, using a short polling interval and
timeout.
In `@tinyagentos/library_store.py`:
- Around line 85-101: Update the library_items CREATE TABLE definition to
declare quality, auto_download, downloaded_at, download_path, and download_bytes
with their existing types and defaults. In _post_init, retain the ALTER TABLE
migration only for legacy databases missing those columns, and commit only when
at least one migration was applied rather than checking col_names, while
preserving existing schema behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 793490c3-123c-4664-941e-416e7481397c
📒 Files selected for processing (7)
tests/test_library.pytinyagentos/library_collections.pytinyagentos/library_pipeline.pytinyagentos/library_store.pytinyagentos/routes/__init__.pytinyagentos/routes/library.pytinyagentos/templates/library.html
…bbit) - Capture create_job id directly instead of racy get_item_jobs()[-1] lookup - Don't flip ready→error on heavy download failure (cheap-tier artifacts stay valid) - Switch WebProcessor to client.stream() for real in-stream size capping - Match file fallback by expected stem, exclude .part files - Gate auto-download on item status==ready (not error) - Remove extraneous f-prefixes from static HTML strings - Add CSRF header injection for HTMX write requests in library page - Assert title in YouTube processor test instead of dead comment - Replace fixed asyncio.sleep with bounded status polling in 3 tests - Fold P3 columns into CREATE TABLE, gate migration commit on actual ALTERs - Update httpx mock to support both client.get() and client.stream()
|
Bot-fix round pushed — addresses all 10 inline findings (4 Kilo WARNING + 6 CodeRabbit actionable) plus 3 CodeRabbit nitpicks: Kilo (4 WARNING):
CodeRabbit (6 actionable): CodeRabbit (3 nitpicks): |
| timeout=httpx.Timeout(30), | ||
| follow_redirects=False, | ||
| ) as client: | ||
| resp = await client.get(current_url) |
There was a problem hiding this comment.
WARNING: Phase 1 client.get() still fully buffers the final URL's body, partially defeating the OOM protection the Phase 2 streaming was added to provide.
This loop uses client.get() (line 413), which downloads and buffers the entire response body. When the final URL is reached (no redirect), the full page is buffered in memory here, then discarded, and Phase 2 re-downloads it via client.stream(). The _MAX_WEB_BYTES cap therefore only guards the second fetch; a hostile or oversized final page can still exhaust memory in Phase 1. Since Phase 1 only needs redirect detection, prefer client.stream() and read only the headers (or issue a streamed/HEAD request) so the body is never buffered here. This also removes the redundant double download.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| candidates = sorted( | ||
| [ | ||
| f for f in download_dir.glob("*") | ||
| if expected_stem in f.name and not f.suffix == ".part" |
There was a problem hiding this comment.
SUGGESTION: Substring stem match may select an unrelated downloaded file.
expected_stem in f.name matches any file whose name contains the expected stem, so a concurrent or similarly-named download (e.g. myvideo vs myvideo_part2) could be picked instead of the intended one. Consider an exact-stem (or extension-aware) match, e.g. f.stem == expected_stem, to avoid mis-selection.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| <title>Library — TinyAgentOS</title> | ||
| <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@picocss/pico@2/css/pico.min.css"> | ||
| <script src="https://unpkg.com/htmx.org@2"></script> | ||
| <meta name="csrf-token" content=""> |
There was a problem hiding this comment.
SUGGESTION: The <meta name="csrf-token" content=""> tag is unused dead markup.
The CSRF token is actually read from the csrf_token cookie (lines 14-17) and injected via hx-headers; this empty meta tag is never populated or read, which is misleading. Remove it, or wire it up if a meta-based token is intended.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_library.py (1)
1089-1096: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract duplicated pipeline polling logic into a shared helper function. The bounded polling loop to wait for the pipeline to reach a terminal status is identically duplicated across three tests, and it currently fails to assert that a terminal status was actually reached before proceeding. If a timeout occurs, tests might fail downstream with confusing error messages.
tests/test_library.py#L1089-L1096: replace this polling loop with a call to a shared helper function that asserts a terminal status was reached.tests/test_library.py#L1121-L1127: replace this polling loop with the shared helper call.tests/test_library.py#L1139-L1145: replace this polling loop with the shared helper call.♻️ Proposed refactor (add helper and replace)
Add a helper function to the test module (or test class):
async def _wait_for_terminal_status(client, item_id, timeout_sec=5.0): import asyncio import pytest iterations = int(timeout_sec / 0.25) for _ in range(iterations): resp = await client.get(f"/api/library/items/{item_id}") if resp.status_code == 200 and resp.json().get("status") in ("ready", "error"): return resp.json()["status"] await asyncio.sleep(0.25) pytest.fail(f"Timeout waiting for pipeline terminal status on item {item_id}")Then replace the duplicated loops in your tests with:
await _wait_for_terminal_status(client, item_id)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_library.py` around lines 1089 - 1096, Extract the duplicated terminal-status polling into a shared async helper, such as _wait_for_terminal_status, that polls for ready or error within the bounded timeout and explicitly fails when no terminal status is reached. Replace the loops at tests/test_library.py lines 1089-1096, 1121-1127, and 1139-1145 with calls to this helper; all three sites require the same direct change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_library.py`:
- Around line 1089-1096: Extract the duplicated terminal-status polling into a
shared async helper, such as _wait_for_terminal_status, that polls for ready or
error within the bounded timeout and explicitly fails when no terminal status is
reached. Replace the loops at tests/test_library.py lines 1089-1096, 1121-1127,
and 1139-1145 with calls to this helper; all three sites require the same direct
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 08deba4a-d024-49f9-aa21-d724194bd929
📒 Files selected for processing (5)
tests/test_library.pytinyagentos/library_pipeline.pytinyagentos/library_store.pytinyagentos/routes/library.pytinyagentos/templates/library.html
🚧 Files skipped from review as they are similar to previous changes (4)
- tinyagentos/templates/library.html
- tinyagentos/library_store.py
- tinyagentos/library_pipeline.py
- tinyagentos/routes/library.py
Phase 1 now uses client.head() for redirect detection, falling back to client.get() if the server rejects HEAD. This prevents the final URL's body from being buffered in memory during redirect resolution — only Phase 2's client.stream() reads the body. Also remove the unused <meta name="csrf-token"> tag; the CSRF token is read from the cookie directly.
| ) as client: | ||
| try: | ||
| resp = await client.head(current_url) | ||
| except Exception: |
There was a problem hiding this comment.
SUGGESTION: Fallback to GET only triggers on an exception, not when the server rejects HEAD with a 405 status.
The comment on line 405 says the code "falls back to GET if the server rejects HEAD", but except Exception here only catches errors raised by client.head() (network failures, timeouts). A server that answers HEAD with 405 Method Not Allowed returns a normal, non-raising response, so resp.is_redirect is False, the loop breaks, and the redirect for that source is silently never followed. Either tighten the comment to "falls back to GET on request error", or also fall back to GET when resp.status_code == 405 (or more generally when the response is neither a redirect nor a 2xx).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
Round 2 pushed — Kilo WARNING resolved (HEAD for redirect resolution). Round 2 bot review: 0 CRITICAL, 0 WARNING. Only 1 SUGGESTION (clarified HEAD comment) + 1 CodeRabbit nitpick (extract polling helper — deferred as low-value refactor). All 71 targeted library tests pass. Ready for maintainer review. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tinyagentos/library_pipeline.py (1)
404-419: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse a streamed GET for the fallback to prevent buffering large payloads.
The current fallback uses
client.get(), which reads the entire response body into memory. If a malicious or oversized site dropsHEADrequests to force this fallback, it bypasses the_MAX_WEB_BYTESprotection completely, potentially causing an out-of-memory (OOM) error. Additionally, catching a blindExceptionsuppresses potential logical errors, and servers that return405 Method Not AllowedforHEADare not handled—breaking redirect traversal for those sites.Catch
httpx.RequestErrorinstead of a blindException(which resolves the static analysis warning) and use a streamedGETto fetch headers without buffering the body. Explicitly trigger this fallback on405responses as well.🔒 Proposed fix to properly stream the GET fallback
- # Phase 1: resolve redirects using HEAD to avoid buffering - # bodies. Falls back to GET on connection/network errors, but - # a server that rejects HEAD (e.g. 405) won't be a redirect - # response either — so the loop exits and Phase 2 streams. + # Phase 1: resolve redirects using HEAD to avoid buffering bodies. + # Falls back to a streamed GET on connection/network errors or if + # the server rejects HEAD (e.g. 405) to ensure redirects can still + # be followed without fully buffering large malicious payloads. current_url = source_url for _hop in range(_MAX_WEB_REDIRECTS + 1): validate_url_or_raise(current_url) async with httpx.AsyncClient( timeout=httpx.Timeout(30), follow_redirects=False, ) as client: - try: - resp = await client.head(current_url) - except Exception: - resp = await client.get(current_url) + try: + resp = await client.head(current_url) + except httpx.RequestError: + resp = None + + if resp is None or resp.status_code == 405: + async with client.stream("GET", current_url) as resp: + pass🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/library_pipeline.py` around lines 404 - 419, Update the redirect-resolution loop around the `client.head` call to catch only `httpx.RequestError`, and trigger the fallback for both request failures and `405 Method Not Allowed` responses. Replace the fallback `client.get` with a streamed GET that reads only the response headers or redirect metadata, closes the response, and preserves `_MAX_WEB_BYTES` enforcement for the later body download.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tinyagentos/library_pipeline.py`:
- Around line 404-419: Update the redirect-resolution loop around the
`client.head` call to catch only `httpx.RequestError`, and trigger the fallback
for both request failures and `405 Method Not Allowed` responses. Replace the
fallback `client.get` with a streamed GET that reads only the response headers
or redirect metadata, closes the response, and preserves `_MAX_WEB_BYTES`
enforcement for the later body download.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 12f46456-f528-4924-a05e-869f4048c38d
📒 Files selected for processing (3)
tests/test_library.pytinyagentos/library_pipeline.pytinyagentos/templates/library.html
💤 Files with no reviewable changes (1)
- tinyagentos/templates/library.html
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_library.py
|
Library P1 is merged (#2062, dev sha 4498855). Three rounds of folds, all closed, including a data-loss fix inside an hour. Good work. This PR is now CONFLICTING against dev and needs a rebase before it can be reviewed. Both P2 and P3 touch the four files P1 substantially rewrote:
Please rebase onto current dev rather than merging dev in, so the diff stays reviewable. Read the merged P1 before resolving, because several things changed underneath you and a mechanical conflict resolution will reintroduce defects that were just fixed:
The canonical envelope reference is taosmd No rush on ordering: P2 first makes sense since P3 builds on it. |
2 participants
Epic #2057 — Phase P3: Heavy tier
Builds on PR #2068 (P2 — YouTube cheap tier + web ingestor).
What this adds
Opt-in media download (heavy tier):
HeavyDownloadProcessorwrapping yt-dlp'sdownload_video()POST /api/library/items/{item_id}/download— trigger heavy downloadGET /api/library/items/{item_id}/download/status— check progressQuality preference settings:
qualitycolumn on library_items (P3 migration, safe ALTER TABLE)Storage accounting:
LibraryStore.get_storage_summary()— total bytes, item count, per-kind breakdownGET /api/library/usageendpoint with HTMX-aware HTML outputPer-source rules engine:
library_rulestable — source_pattern (fnmatch), quality, auto_download, enabledPOST/GET/DELETE /api/library/rules— CRUD endpointsLibraryStore.match_rules(source_url)— fnmatch-based matching<details>panel)_ingest_taskchecks matching rules withauto_download=Trueand triggers heavy pipeline after cheap tier completesFiles changed (5)
tinyagentos/library_store.pytinyagentos/library_pipeline.pytinyagentos/routes/library.pytinyagentos/templates/library.htmltests/test_library.pyTest gate
test_filter_by_kind) skipped — hits real YouTube APIDesign doc
Per
docs/design/library-app.mdsection 3 (heavy tier) and section 4 step 5.Summary by CodeRabbit