feat(library): P2 — YouTube cheap-tier processor + generic web-page ingestor - #2068
feat(library): P2 — YouTube cheap-tier processor + generic web-page ingestor#2068hognek wants to merge 6 commits into
Conversation
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? |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds YouTube and web URL processors, routes URL-only items through them, introduces library ingestion and item lifecycle endpoints, and adds asynchronous tests covering artifacts, metadata, titles, missing inputs, and pipeline status transitions. ChangesLibrary ingestion
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Client
participant LibraryRoutes
participant LibraryStore
participant run_pipeline
participant URLProcessor
Client->>LibraryRoutes: Submit URL for ingestion
LibraryRoutes->>LibraryStore: Create pending item
LibraryRoutes->>run_pipeline: Schedule background processing
run_pipeline->>URLProcessor: Process YouTube or web URL
URLProcessor->>LibraryStore: Save generated artifacts
run_pipeline->>LibraryStore: Mark item ready
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
| return ( | ||
| f'<div class="item-card" id="item-{item_id}">' | ||
| f'<div class="info">' | ||
| f'<h3>{title}</h3>' |
There was a problem hiding this comment.
CRITICAL: Reflected XSS — title (and item_id at line 250, kind at 255, status at 254) are interpolated unescaped into HTML.
title originates from untrusted input: the web page <title> tag (WebProcessor), YouTube metadata, or the user-supplied title form field. An attacker who controls a fetched page title (e.g. <title><img src=x onerror=alert(1)></title>) or submits a crafted title value can inject HTML/JS into the library item list, executed in any admin session that views /api/library/items via HTMX.
Escape all interpolated values (e.g. markupsafe.escape / html.escape) before building the fragment.
| f'<h3>{title}</h3>' | |
| f'<h3>{escape(title)}</h3>' |
(You will need to from markupsafe import escape — apply it to item_id, kind, and status as well.)
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| r"<title[^>]*>([^<]+)</title>", html, re.IGNORECASE, | ||
| ) | ||
| if m: | ||
| title = m.group(1).strip()[:200] |
There was a problem hiding this comment.
WARNING: The extracted <title> text is stored raw (no HTML entity unescaping) and later rendered unescaped in _render_item_card. A page whose title contains HTML entities or markup will (a) yield a garbled title and (b) become the XSS vector described on routes/library.py:252.
Additionally m.group(1) includes the raw inner text but the regex <title[^>]*>([^<]+)</title> only captures up to the first <, so a title containing a < (or HTML entities like &) is captured incorrectly. At minimum, run the captured string through html.unescape(...) before storing.
| title = m.group(1).strip()[:200] | |
| title = html.unescape(m.group(1)).strip()[:200] |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # Stream file in bounded chunks to avoid loading it entirely into | ||
| # memory. Reject uploads exceeding 100 MB with HTTP 413. | ||
| MAX_SIZE = 100 * 1024 * 1024 # 100 MB | ||
| if file.size and file.size > MAX_SIZE: |
There was a problem hiding this comment.
SUGGESTION: file.size is client-reported and frequently None/0 for streamed/chunked multipart uploads, so this pre-check is skipped and the only real protection is the in-loop size > MAX_SIZE counter — which is correct, but the pre-check gives a false sense of defense and will not catch oversized uploads when file.size is missing. Consider relying solely on (and keeping) the in-loop bound, or dropping the pre-check, to avoid implying the cap is enforced up-front.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| except ImportError: | ||
| logger.warning("httpx not available — skipping web fetch for %s", | ||
| source_url) | ||
| return artifacts |
There was a problem hiding this comment.
WARNING: A failed fetch for URL items is swallowed: WebProcessor/YouTubeProcessor catch all exceptions and return artifacts (empty). run_pipeline then still calls update_item_status(item_id, "ready"), so a transient network error, DNS failure, or blocked host leaves the item permanently ready with zero artifacts and no error meta. The pipeline-integration tests even assert ready on a no-op fetch. This makes failed ingests indistinguishable from successful ones and there is no retry.
Consider tracking whether any artifact was produced (or the fetch succeeded) for url:* kinds and marking the item error when the source could not be retrieved, consistent with the existing missing-file error handling for file items.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (7 files)
Previous Review Summaries (6 snapshots, latest commit c7f1020)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit c7f1020)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (5 files)
Fix these issues in Kilo Cloud Previous review (commit c8f6416)Verdict: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)
🏆 Best part: The WebProcessor SSRF redirect hardening is genuinely solid — manual per-hop validation against the blocklist, 10 MB streaming cap, content-type gating. This mirrors 💀 Worst part: The content-type gate at 📊 Overall: Like a bouncer who checks IDs at the door but then lets anyone with a fake mustache into the VIP section — the perimeter is hardened but the exit criteria are confused. Fix the Files Reviewed (5 files)
Fix these issues in Kilo Cloud Previous review (commit 52c3c7a)Status: No New Issues Found (incremental) | Recommendation: Merge OverviewThis incremental review covers commits after
Notes (out of incremental scope)
Files Reviewed (2 files in increment)
Previous review (commit 3ce9d3c)Status: No New Issues Found (incremental) | Recommendation: Merge OverviewThis incremental review covers commits after
Notes (out of incremental scope)
Files Reviewed (2 files in increment)
Previous review (commit e08f98a)Status: No New Issues Found (incremental) | Recommendation: Merge OverviewThis incremental review covers commits after
Resolved in this increment
Notes (out of incremental scope)
Files Reviewed (2 files in increment)
Previous review (commit 96abf8b)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (7 files)
Reviewed by step-3.7-flash · Input: 82.6K · Output: 9.8K · Cached: 984.8K |
![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 (1)
tinyagentos/library_pipeline.py (1)
407-409: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo response-size cap on fetched HTML.
resp.textmaterializes the entire body into memory with only a timeout guarding the call. A large/hostile page can drive high memory use per ingest. Consider streaming with a max-bytes limit (e.g. viaclient.streamand aborting past a threshold) or checkingContent-Length.🤖 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 407 - 409, Update the HTML-fetching flow around client.get and resp.text to enforce a maximum response size before materializing the body. Prefer streaming through the existing client, accumulate only up to the configured byte limit, and abort or reject responses exceeding it; use Content-Length as an early check when available while preserving normal HTML parsing for responses within the limit.
🤖 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_collections.py`:
- Around line 60-118: Track artifacts only after their copy to item_dir
succeeds, and build the manifest’s artifacts list from that successfully copied
collection instead of the original text_artifacts list. Update the per-artifact
loop and manifest construction in the surrounding collection function while
preserving ingestion behavior and skipping missing or failed-copy sources.
In `@tinyagentos/library_pipeline.py`:
- Around line 401-409: Update the HTTP fetch flow around validate_url_or_raise
and httpx.AsyncClient so redirects cannot bypass SSRF validation: disable
automatic redirects and manually inspect each redirect Location, validating its
resolved target with validate_url_or_raise before issuing the next request.
Preserve the existing response handling for non-redirect responses.
In `@tinyagentos/routes/library.py`:
- Around line 79-90: The lazy initialization in _get_library_store is vulnerable
to concurrent requests creating multiple LibraryStore instances across the await
store.init() boundary. Add and reuse an application-scoped asyncio lock to guard
the check, initialization, and assignment, rechecking
request.app.state.library_store after acquiring the lock so only one initialized
store is created and returned.
- Around line 239-271: Update _render_item_card to HTML-escape every
interpolated item field, including item_id, title, status, kind, and size_str,
before inserting them into the raw HTML fragment. Preserve the existing
defaults, status-class lookup, formatting, and _render_item_list behavior while
ensuring both ingest responses and polled lists cannot render
attacker-controlled markup.
In `@tinyagentos/templates/library.html`:
- Around line 75-91: Update the ingest requests in the library template,
including the form’s hx-post and the drop-zone hx-post, to send an X-CSRF-Token
header whose value matches the csrf_token cookie required by verify_csrf. Use
the existing client-side CSRF token mechanism if available, and ensure both
upload paths include the header without changing their request behavior.
- Around line 66-96: Remove the htmx request attributes from the outer
div.drop-zone, including hx-post, hx-encoding, hx-target, hx-swap, and
hx-indicator. Keep the corresponding attributes on form#ingest-form so the
existing direct drop-handler submission remains the sole ingest request.
---
Nitpick comments:
In `@tinyagentos/library_pipeline.py`:
- Around line 407-409: Update the HTML-fetching flow around client.get and
resp.text to enforce a maximum response size before materializing the body.
Prefer streaming through the existing client, accumulate only up to the
configured byte limit, and abort or reject responses exceeding it; use
Content-Length as an early check when available while preserving normal HTML
parsing for responses within the limit.
🪄 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: 51c9530a-15d8-4af6-8185-1498414c1f72
📒 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
| <form class="ingest-form" | ||
| id="ingest-form" | ||
| hx-post="/api/library/ingest" | ||
| hx-encoding="multipart/form-data" | ||
| hx-target="#item-list" | ||
| hx-swap="afterbegin" | ||
| hx-indicator="#ingest-spinner"> | ||
| <input type="file" name="file" id="file-input" style="display:none" | ||
| onchange="document.getElementById('file-name').textContent = this.files[0]?.name || ''"> | ||
| <button type="button" class="secondary outline" | ||
| onclick="document.getElementById('file-input').click()"> | ||
| Choose File | ||
| </button> | ||
| <span id="file-name" style="font-size:0.85rem; align-self:center"></span> | ||
| <input type="text" name="url" placeholder="Or paste a URL…" style="max-width:300px"> | ||
| <button type="submit">Ingest</button> | ||
| </form> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'def verify_csrf' -A 25 tinyagentos/middleware/csrf.py
rg -n 'csrf' -i tests/conftest.pyRepository: jaylfc/taOS
Length of output: 2381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the library template and nearby scripts for any CSRF header injection.
sed -n '1,220p' tinyagentos/templates/library.html
printf '\n--- CSRF-related occurrences ---\n'
rg -n 'X-CSRF-Token|csrf_token|hx-headers|htmx:configRequest|setRequestHeader|verify_csrf' tinyagentos -g '!**/*.pyc'
printf '\n--- Test-mode bypass context ---\n'
sed -n '1,120p' tests/conftest.pyRepository: jaylfc/taOS
Length of output: 11223
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any global client-side code that injects CSRF headers into htmx/fetch requests.
rg -n 'htmx:configRequest|X-CSRF-Token|csrf_token|hx-headers|setRequestHeader|beforeRequest|fetch\(' tinyagentos -g '!**/*.pyc'
printf '\n--- App/template JS files ---\n'
git ls-files 'tinyagentos/**/*.js' 'tinyagentos/**/*.html' 'tinyagentos/**/*.ts' | sed -n '1,200p'Repository: jaylfc/taOS
Length of output: 1858
Add X-CSRF-Token to the ingest request
verify_csrf requires the X-CSRF-Token header to match the csrf_token cookie, but this template never sets it for either the form or the drop-zone hx-post. The route tests bypass CSRF in tests/conftest.py, so this will fail in normal use.
🤖 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/templates/library.html` around lines 75 - 91, Update the ingest
requests in the library template, including the form’s hx-post and the drop-zone
hx-post, to send an X-CSRF-Token header whose value matches the csrf_token
cookie required by verify_csrf. Use the existing client-side CSRF token
mechanism if available, and ensure both upload paths include the header without
changing their request behavior.
|
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. |
…nt TOCTOU race Two concurrent requests that both see library_store as None could create and init two separate LibraryStore instances, leaking connections and racing on early writes. Add _store_init_lock (module-level asyncio.Lock) with double-checked pattern — same approach as _config_lock in config.py. Addresses CodeRabbit finding from PR jaylfc#2068 review cycle.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tinyagentos/routes/library.py (1)
167-175: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSkip collections handoff when the pipeline ended in error.
run_pipelinecatches processor exceptions, marks the itemerror, and returns normally. Consequently, this outertrysucceeds and can index partial artifacts produced before the failure.Proposed fix
await run_pipeline(store, item_id, storage_dir) + processed_item = await store.get_item(item_id) + if not processed_item or processed_item["status"] != "ready": + return🤖 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/routes/library.py` around lines 167 - 175, Update the ingest flow around run_pipeline so it communicates processor failure to the caller, then make the collections handoff execute only when the pipeline completes successfully. Preserve the existing error status update and exception logging, and ensure partial artifacts are not handed off after run_pipeline catches an error.tinyagentos/library_collections.py (1)
80-127: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRemove stale collection files during reprocessing.
item_dirpersists across runs, but only current artifacts are copied. If a transcript or OCR artifact disappears—or there are no text artifacts on the next run—the old file remains available to taosmd and can continue being indexed.Rebuild the per-item directory from the successfully copied current artifacts before indexing, including the empty-artifact case.
🤖 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_collections.py` around lines 80 - 127, Update the artifact-processing flow around item_dir and indexed_paths to rebuild the per-item directory on every run: remove stale contents before copying current text artifacts, including clearing it when text_artifacts is empty. Preserve only files successfully copied from the current artifacts, then continue indexing from the rebuilt directory.
🧹 Nitpick comments (2)
tests/test_library.py (2)
523-531: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winExercise the SSRF guard instead of only bypassing it.
The success tests mock
validate_url_or_raisebut never assert it was called. Add assertions plus blocked-URL and redirect-target cases so removal of per-hop validation cannot pass unnoticed.🤖 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 523 - 531, Add assertions in the tests around proc.process(item) to verify tinyagentos.routes.desktop_browser.ssrf.validate_url_or_raise is called with the expected URL. Add coverage for blocked URLs and redirect targets, asserting both are rejected, so per-hop SSRF validation is exercised rather than bypassed by the mock.
932-961: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSynchronize with background-task completion instead of sleeping.
The
0.5-second assumptions are CI-dependent, and the second reprocess is never awaited or verified. Poll for a terminal status with a timeout—or await the tracked task—before each assertion. Construct the conflict test directly inprocessingstate rather than racing an external URL fetch.Also applies to: 969-983
🤖 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 932 - 961, Replace the fixed asyncio.sleep calls in the reprocessing test with polling for the item’s terminal status, using a bounded timeout before checking artifact counts or readiness. Await and verify the second /reprocess request completes without duplicating artifacts, and update the conflict test to create the item directly in processing state instead of racing an external URL fetch.
🤖 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 `@tests/test_library.py`:
- Around line 415-419: Update the test around updated_title to assert that the
processed item’s title matches the expected YouTube video title, rather than
only retrieving the field. Remove the non-assertive comments and preserve the
existing update_item flow.
In `@tinyagentos/library_collections.py`:
- Around line 326-349: Validate the response returned by the collection-linking
POST in the linking flow before logging success or clearing
collection_retryable. Treat non-success HTTP statuses, including 401, 404, and
500, as failures and route them through the existing warning/error handling so
retry state is preserved; only perform the “Linked collection” log and metadata
update after confirmed success.
In `@tinyagentos/library_pipeline.py`:
- Around line 648-665: Remove the URL-only reference-artifact branch surrounding
the storage_path/source_url checks, including its logger message, ref_meta
construction, and store.add_artifact call. Allow registered processors to handle
URL-only YouTube/web items and fetch their content; only retain equivalent
handling if the implementation explicitly identifies genuinely unsupported URL
kinds.
- Around line 288-290: Update the fetch flow used by the media retrieval call to
impose a per-process timeout on each yt-dlp subprocess, and ensure timed-out or
cancelled processes are terminated, force-killed if necessary, and awaited so no
child remains running. Preserve normal successful fetch behavior and apply
cleanup consistently to all three yt-dlp invocations.
- Around line 415-422: Update the redirect-fetch loop around
validate_url_or_raise and client.get so the validated DNS address is pinned for
the connection, preventing a second hostname resolution; fetch using the pinned
IP while preserving the original Host header and TLS SNI, or alternatively
verify the connected peer address matches the validated result before accepting
the response.
- Around line 418-422: Update the web-fetch flow around httpx.AsyncClient and
client.get to use client.stream() so the response body is consumed
incrementally; enforce _MAX_WEB_BYTES during aiter_bytes() iteration before
accumulating content, while preserving the existing redirect and timeout
settings.
In `@tinyagentos/routes/library.py`:
- Around line 312-355: Make the reprocess flow around try_update_item_status and
artifact cleanup failure-safe: wrap destructive artifact deletion and task
scheduling in preparation error handling, transition the item to error on any
cleanup or scheduling failure, and do not roll back to ready after artifacts are
removed. Return the success response only after _track_background_task or
_create_supervised_task has successfully established task ownership.
- Around line 177-200: Update the item-processing lifecycle around run_pipeline
and handoff_to_collections so the per-item task/lock remains registered until
the collections handoff completes, rather than ending when the item is marked
ready. Make the reprocess endpoint consult that same active per-item state and
reject requests while either pipeline or handoff work is running, preserving
normal reprocessing once the lifecycle finishes.
---
Outside diff comments:
In `@tinyagentos/library_collections.py`:
- Around line 80-127: Update the artifact-processing flow around item_dir and
indexed_paths to rebuild the per-item directory on every run: remove stale
contents before copying current text artifacts, including clearing it when
text_artifacts is empty. Preserve only files successfully copied from the
current artifacts, then continue indexing from the rebuilt directory.
In `@tinyagentos/routes/library.py`:
- Around line 167-175: Update the ingest flow around run_pipeline so it
communicates processor failure to the caller, then make the collections handoff
execute only when the pipeline completes successfully. Preserve the existing
error status update and exception logging, and ensure partial artifacts are not
handed off after run_pipeline catches an error.
---
Nitpick comments:
In `@tests/test_library.py`:
- Around line 523-531: Add assertions in the tests around proc.process(item) to
verify tinyagentos.routes.desktop_browser.ssrf.validate_url_or_raise is called
with the expected URL. Add coverage for blocked URLs and redirect targets,
asserting both are rejected, so per-hop SSRF validation is exercised rather than
bypassed by the mock.
- Around line 932-961: Replace the fixed asyncio.sleep calls in the reprocessing
test with polling for the item’s terminal status, using a bounded timeout before
checking artifact counts or readiness. Await and verify the second /reprocess
request completes without duplicating artifacts, and update the conflict test to
create the item directly in processing state instead of racing an external URL
fetch.
🪄 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: b55a9e94-cf49-4081-ba87-5eac9ed94031
📒 Files selected for processing (6)
.gitignoretests/test_library.pytinyagentos/library_collections.pytinyagentos/library_pipeline.pytinyagentos/library_store.pytinyagentos/routes/library.py
| # 4. Link the collection to the library item. | ||
| try: | ||
| await http_client.post( | ||
| f"{base_url}/collections/{collection_id}/link", | ||
| json={"type": "taos", "id": item_id}, | ||
| headers=auth_headers, | ||
| ) | ||
| logger.debug( | ||
| "Linked collection %s to library item %s", | ||
| collection_id, item_id, | ||
| ) | ||
| except Exception: | ||
| logger.warning( | ||
| "taosmd POST /collections/%s/link failed for item %s", | ||
| collection_id, item_id, exc_info=True, | ||
| ) | ||
|
|
||
| # Clear retryable on success; the handoff completed. | ||
| if indexed > 0: | ||
| item_meta.pop("collection_retryable", None) | ||
| try: | ||
| await store.update_item(item_id, meta_json=item_meta) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate the collection-link response before declaring success.
HTTP 401/404/500 responses do not raise here, so the code logs “Linked,” clears retry state, and returns an indexed count although the collection was never linked.
Proposed fix
- await http_client.post(
+ link_resp = await http_client.post(
f"{base_url}/collections/{collection_id}/link",
json={"type": "taos", "id": item_id},
headers=auth_headers,
)
+ if link_resp.status_code >= 400:
+ logger.warning(
+ "taosmd POST /collections/%s/link returned %d",
+ collection_id, link_resp.status_code,
+ )
+ await _set_retryable()
+ return 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # 4. Link the collection to the library item. | |
| try: | |
| await http_client.post( | |
| f"{base_url}/collections/{collection_id}/link", | |
| json={"type": "taos", "id": item_id}, | |
| headers=auth_headers, | |
| ) | |
| logger.debug( | |
| "Linked collection %s to library item %s", | |
| collection_id, item_id, | |
| ) | |
| except Exception: | |
| logger.warning( | |
| "taosmd POST /collections/%s/link failed for item %s", | |
| collection_id, item_id, exc_info=True, | |
| ) | |
| # Clear retryable on success; the handoff completed. | |
| if indexed > 0: | |
| item_meta.pop("collection_retryable", None) | |
| try: | |
| await store.update_item(item_id, meta_json=item_meta) | |
| except Exception: | |
| pass | |
| # 4. Link the collection to the library item. | |
| try: | |
| link_resp = await http_client.post( | |
| f"{base_url}/collections/{collection_id}/link", | |
| json={"type": "taos", "id": item_id}, | |
| headers=auth_headers, | |
| ) | |
| if link_resp.status_code >= 400: | |
| logger.warning( | |
| "taosmd POST /collections/%s/link returned %d", | |
| collection_id, link_resp.status_code, | |
| ) | |
| await _set_retryable() | |
| return 0 | |
| logger.debug( | |
| "Linked collection %s to library item %s", | |
| collection_id, item_id, | |
| ) | |
| except Exception: | |
| logger.warning( | |
| "taosmd POST /collections/%s/link failed for item %s", | |
| collection_id, item_id, exc_info=True, | |
| ) | |
| # Clear retryable on success; the handoff completed. | |
| if indexed > 0: | |
| item_meta.pop("collection_retryable", None) | |
| try: | |
| await store.update_item(item_id, meta_json=item_meta) | |
| except Exception: | |
| pass |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 337-337: Do not catch blind exception: Exception
(BLE001)
[error] 348-349: try-except-pass detected, consider logging the exception
(S110)
[warning] 348-348: Do not catch blind exception: Exception
(BLE001)
🤖 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_collections.py` around lines 326 - 349, Validate the
response returned by the collection-linking POST in the linking flow before
logging success or clearing collection_retryable. Treat non-success HTTP
statuses, including 401, 404, and 500, as failures and route them through the
existing warning/error handling so retry state is preserved; only perform the
“Linked collection” log and metadata update after confirmed success.
| media_dir = self.storage_dir / "youtube" | ||
| result = await fetch(source_url, media_dir=media_dir) | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline tinyagentos/knowledge_fetchers/youtube.py \
--items all --type function --match fetch --view expanded
rg -nP -C3 '\b(create_subprocess_exec|communicate|wait_for|timeout|terminate|kill)\b' \
tinyagentos/knowledge_fetchers/youtube.py tinyagentos/library_pipeline.pyRepository: jaylfc/taOS
Length of output: 5313
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## tinyagentos/knowledge_fetchers/youtube.py outline\n'
ast-grep outline tinyagentos/knowledge_fetchers/youtube.py --items all --type function --match fetch --view expanded
printf '\n## tinyagentos/knowledge_fetchers/youtube.py relevant ranges\n'
sed -n '110,210p' tinyagentos/knowledge_fetchers/youtube.py | cat -n
printf '\n## tinyagentos/library_pipeline.py call site and surrounding logic\n'
sed -n '270,310p' tinyagentos/library_pipeline.py | cat -nRepository: jaylfc/taOS
Length of output: 6735
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('tinyagentos/knowledge_fetchers/youtube.py')
text = p.read_text()
# probe for cancellation / cleanup patterns
for needle in ['wait_for(', 'asyncio.timeout', 'terminate(', 'kill(', 'try:', 'CancelledError', 'finally:']:
print(f'{needle}:', needle in text)
PYRepository: jaylfc/taOS
Length of output: 269
Bound and clean up the yt-dlp subprocesses.
fetch() waits on three yt-dlp invocations with no timeout or cancellation cleanup; a hung child can stall ingestion indefinitely. Add per-process deadlines and terminate/kill/reap timed-out children.
🤖 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 288 - 290, Update the fetch
flow used by the media retrieval call to impose a per-process timeout on each
yt-dlp subprocess, and ensure timed-out or cancelled processes are terminated,
force-killed if necessary, and awaited so no child remains running. Preserve
normal successful fetch behavior and apply cleanup consistently to all three
yt-dlp invocations.
| 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: | ||
| resp = await client.get(current_url) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline tinyagentos/routes/desktop_browser/ssrf.py \
--items all --type function,class \
--match 'validate_url_or_raise|validate_resolved_addr|SsrfBlockedError'
rg -nP -C3 \
'getaddrinfo|validate_url_or_raise|AsyncClient|AsyncHTTPTransport|network_stream|get_extra_info' \
tinyagentosRepository: jaylfc/taOS
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- tinyagentos/routes/desktop_browser/ssrf.py ---'
sed -n '1,220p' tinyagentos/routes/desktop_browser/ssrf.py
echo
echo '--- tinyagentos/library_pipeline.py (390-460) ---'
sed -n '390,460p' tinyagentos/library_pipeline.py
echo
echo '--- tinyagentos/routes/userspace_apps.py (150-175) ---'
sed -n '150,175p' tinyagentos/routes/userspace_apps.pyRepository: jaylfc/taOS
Length of output: 10653
🏁 Script executed:
#!/bin/bash
echo hiRepository: jaylfc/taOS
Length of output: 152
Pin the validated IP before fetching tinyagentos/library_pipeline.py:416-422 only validates the DNS result once; client.get(current_url) resolves the hostname again on connect, leaving a DNS-rebinding SSRF gap. Resolve once and fetch the pinned IP while preserving the original Host/SNI, or verify the peer address from the socket.
🤖 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 415 - 422, Update the
redirect-fetch loop around validate_url_or_raise and client.get so the validated
DNS address is pinned for the connection, preventing a second hostname
resolution; fetch using the pinned IP while preserving the original Host header
and TLS SNI, or alternatively verify the connected peer address matches the
validated result before accepting the response.
| # URL-only items (no storage_path) are stored as references — the pipeline | ||
| # records a reference metadata artifact but does not fetch remote content | ||
| # (future: WebFetcherProcessor, #2078). The item gets a "reference" artifact so it | ||
| # is not silently empty. | ||
| if not item.get("storage_path") and item.get("source_url"): | ||
| logger.info( | ||
| "Library pipeline: URL-only item %s (%s) — stored as reference, not fetched", | ||
| item_id, kind, | ||
| ) | ||
| ref_meta = { | ||
| "source_url": item["source_url"], | ||
| "kind": kind, | ||
| "note": "Reference-only item — content not fetched (TODO: WebFetcherProcessor)", | ||
| } | ||
| await store.add_artifact( | ||
| item_id, kind="reference", path="", meta=ref_meta, | ||
| ) | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Remove the obsolete reference-only artifact.
Every URL-only YouTube/web item records “content not fetched,” then its registered processor fetches the content. Remove this block or restrict it to genuinely unsupported URL kinds.
🤖 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 648 - 665, Remove the URL-only
reference-artifact branch surrounding the storage_path/source_url checks,
including its logger message, ref_meta construction, and store.add_artifact
call. Allow registered processors to handle URL-only YouTube/web items and fetch
their content; only retain equivalent handling if the implementation explicitly
identifies genuinely unsupported URL kinds.
| # Atomic CAS: transition to pending only when NOT already pending/processing. | ||
| # Beats the TOCTOU race — two concurrent reprocess requests cannot both | ||
| # pass a separate read-then-write guard. | ||
| if not await store.try_update_item_status(item_id, "pending", | ||
| if_not_in=("pending", "processing")): | ||
| return JSONResponse( | ||
| {"error": "Item is currently being processed"}, status_code=409 | ||
| ) | ||
|
|
||
| # Delete old artifacts so reprocess is idempotent. | ||
| # Guard: never unlink the user's original uploaded file (storage_path). | ||
| storage_dir = _library_dir(request) | ||
| old_artifacts = await store.get_artifacts(item_id) | ||
| item_storage_path = item.get("storage_path", "") | ||
| for art in old_artifacts: | ||
| art_path = art.get("path", "") | ||
| if art_path and art_path == item_storage_path: | ||
| # This artifact records the source file — skip deletion. | ||
| logger.debug("Skipping unlink of source file %s for item %s", | ||
| art_path, item_id) | ||
| elif art_path and (ap := Path(art_path)).exists(): | ||
| try: | ||
| ap.unlink() | ||
| except OSError: | ||
| logger.warning("Failed to remove artifact %s for item %s", | ||
| art_path, item_id) | ||
| await store.delete_artifact(art["id"]) | ||
|
|
||
| # Status was already atomically set to pending by try_update_item_status above; | ||
| # re-queue the pipeline now. If scheduling fails, roll back to ready so the | ||
| # item is not stuck pending forever. | ||
| try: | ||
| task_set = getattr(request.app.state, "_background_tasks", None) | ||
| coro = _ingest_task(request.app, item_id, store, storage_dir) | ||
| if task_set is None: | ||
| _track_background_task(coro) | ||
| else: | ||
| _create_supervised_task(coro, task_set) | ||
| except Exception: | ||
| logger.exception("Failed to schedule reprocess for item %s", item_id) | ||
| await store.update_item_status(item_id, "ready") | ||
| return JSONResponse( | ||
| {"error": "Failed to schedule reprocess"}, status_code=500, | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make destructive reprocess preparation failure-safe.
After the CAS sets pending, any cleanup exception strands the item partially deleted and pending. If scheduling fails, rolling back to ready is also incorrect because its artifacts have already been removed. Catch preparation failures and move the item to error; only expose a successful queued state once task ownership is established.
🤖 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/routes/library.py` around lines 312 - 355, Make the reprocess
flow around try_update_item_status and artifact cleanup failure-safe: wrap
destructive artifact deletion and task scheduling in preparation error handling,
transition the item to error on any cleanup or scheduling failure, and do not
roll back to ready after artifacts are removed. Return the success response only
after _track_background_task or _create_supervised_task has successfully
established task ownership.
…nt TOCTOU race Two concurrent requests that both see library_store as None could create and init two separate LibraryStore instances, leaking connections and racing on early writes. Add _store_init_lock (module-level asyncio.Lock) with double-checked pattern — same approach as _config_lock in config.py. Addresses CodeRabbit finding from PR jaylfc#2068 review cycle.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_library.py (2)
883-908: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDocstring claims 6 endpoints, only 5 are tested.
Both the class comment (Line 885-886) and docstring (Line 890) say "6 … endpoints", but
endpointsonly lists 5 entries (Line 894-900). Either the count is stale or a 6th endpoint's auth check is missing.🤖 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 883 - 908, Align the unauthenticated endpoint test’s count with the actual coverage: inspect the library routes and either add the missing sixth endpoint to the endpoints list in test_unauth_library_endpoints or update the surrounding comment and docstring to state five endpoints. Keep the 401 assertion behavior unchanged for every listed route.
817-822: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winStub library ingest fetchers in
TestLibraryRoutes.
test_ingest_url,test_ingest_and_get,test_filter_by_kind, andtest_reprocess_while_processing_returns_409queuerun_pipeline()in the background, andWebProcessor/YouTubeProcessorstill hit realhttpx.AsyncClientandyt-dlpcalls. Add a shared autouse fixture to patch those fetch paths so these cases stay deterministic.🤖 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 817 - 822, Update TestLibraryRoutes with a shared autouse fixture that stubs the WebProcessor and YouTubeProcessor fetch paths used by background run_pipeline() execution, preventing real httpx.AsyncClient and yt-dlp calls. Ensure the fixture applies to test_ingest_url, test_ingest_and_get, test_filter_by_kind, and test_reprocess_while_processing_returns_409 while preserving their existing assertions and pipeline behavior.
🧹 Nitpick comments (4)
tests/test_library.py (4)
706-793: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant local re-imports of already module-level names.
AsyncMock/patch/MagicMockare imported locally at Line 709 and 727, duplicating the module-level import at Line 8.🤖 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 706 - 793, Remove the redundant local imports of AsyncMock, patch, and MagicMock from test_handoff_with_qmd; reuse the existing module-level imports while leaving the test setup and behavior unchanged.
344-344: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
next(...)over indexing a full list comprehension.Flagged by Ruff RUF015.
Proposed fix
- thumb_art = [a for a in artifacts if a["kind"] == "thumbnail"][0] + thumb_art = next(a for a in artifacts if a["kind"] == "thumbnail")🤖 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` at line 344, Update the thumbnail lookup in the artifact-selection test to use next(...) over a generator expression instead of building and indexing a full list comprehension, while preserving selection of the first artifact whose kind is "thumbnail".Source: Linters/SAST tools
499-544: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winNo test coverage for
WebProcessor's redirect-loop / SSRF revalidation.
WebProcessor.processimplements a bounded redirect loop that re-validates each hop against the SSRF blocklist per "disable auto-redirects, manually validate each hop against the SSRF blocklist, and cap total response bytes" — a security-sensitive path that's entirely untested here (_mock_httpx_responsehardcodesis_redirect = False, and_MAX_WEB_REDIRECTS/size-cap paths aren't exercised). Consider extending_mock_httpx_streamto return a redirect response followed by a final response to cover this.🤖 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 499 - 544, The WebProcessor tests do not cover manual redirect handling, per-hop SSRF validation, or redirect limits. Extend test_process_web_url and the _mock_httpx_stream setup to return a redirect response followed by a final response, assert each hop is validated and the final content is processed, and add coverage for the bounded redirect/response-size behavior using the existing _MAX_WEB_REDIRECTS and size-cap paths.
913-956: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFixed
asyncio.sleep(0.5)to await background pipeline completion is inherently flaky.Both
test_reprocess_idempotentandtest_reprocess_while_processing_returns_409assume the background pipeline finishes within 500ms. Under CI load or slower I/O this can race and produce intermittent failures/false negatives (e.g., asserting artifact counts or status before the pipeline actually completes).♻️ Suggested polling helper instead of fixed sleep
- # Wait for pipeline to finish (background task) - import asyncio - await asyncio.sleep(0.5) + # Wait for pipeline to finish (background task), polling instead of a fixed sleep + import asyncio + for _ in range(20): + resp = await client.get(f"/api/library/items/{item_id}") + if resp.json()["item"].get("status") in ("ready", "error"): + break + await asyncio.sleep(0.1)Also applies to: 958-978
🤖 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 913 - 956, Replace the fixed asyncio.sleep(0.5) waits in test_reprocess_idempotent and test_reprocess_while_processing_returns_409 with polling that repeatedly fetches the item until the expected pipeline status or artifacts are available, using a bounded timeout and short interval. Preserve the existing assertions and 409-in-progress behavior while making completion checks reliable under slower CI conditions.
🤖 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 418-447: Move resp.raise_for_status() and the response body
consumption into the async with client.stream context in the redirect loop.
Preserve redirect handling and only break after the final response has been
validated and fully read, storing the decoded body for subsequent processing.
---
Outside diff comments:
In `@tests/test_library.py`:
- Around line 883-908: Align the unauthenticated endpoint test’s count with the
actual coverage: inspect the library routes and either add the missing sixth
endpoint to the endpoints list in test_unauth_library_endpoints or update the
surrounding comment and docstring to state five endpoints. Keep the 401
assertion behavior unchanged for every listed route.
- Around line 817-822: Update TestLibraryRoutes with a shared autouse fixture
that stubs the WebProcessor and YouTubeProcessor fetch paths used by background
run_pipeline() execution, preventing real httpx.AsyncClient and yt-dlp calls.
Ensure the fixture applies to test_ingest_url, test_ingest_and_get,
test_filter_by_kind, and test_reprocess_while_processing_returns_409 while
preserving their existing assertions and pipeline behavior.
---
Nitpick comments:
In `@tests/test_library.py`:
- Around line 706-793: Remove the redundant local imports of AsyncMock, patch,
and MagicMock from test_handoff_with_qmd; reuse the existing module-level
imports while leaving the test setup and behavior unchanged.
- Line 344: Update the thumbnail lookup in the artifact-selection test to use
next(...) over a generator expression instead of building and indexing a full
list comprehension, while preserving selection of the first artifact whose kind
is "thumbnail".
- Around line 499-544: The WebProcessor tests do not cover manual redirect
handling, per-hop SSRF validation, or redirect limits. Extend
test_process_web_url and the _mock_httpx_stream setup to return a redirect
response followed by a final response, assert each hop is validated and the
final content is processed, and add coverage for the bounded
redirect/response-size behavior using the existing _MAX_WEB_REDIRECTS and
size-cap paths.
- Around line 913-956: Replace the fixed asyncio.sleep(0.5) waits in
test_reprocess_idempotent and test_reprocess_while_processing_returns_409 with
polling that repeatedly fetches the item until the expected pipeline status or
artifacts are available, using a bounded timeout and short interval. Preserve
the existing assertions and 409-in-progress behavior while making completion
checks reliable under slower CI conditions.
🪄 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: 54ffe829-c02a-4f03-aa80-1b824889963b
📒 Files selected for processing (3)
tests/test_library.pytinyagentos/library_pipeline.pytinyagentos/routes/library.py
|
Noted that #2085 was opened for the same content and I have closed it as superseded by this PR, since both pointed at For the next round: fold findings into the PR that has them rather than opening a sibling. The rebase onto the merged P1 was the right move and this branch has it. I will review this properly once the CI matrix settles. Worth knowing before then: the base moved substantially under you when P1 merged, so please confirm the conflict resolution preserved P1's four fixes rather than reverting any of them. The one that matters most is the guard that stops reprocess unlinking the item's |
|
Reviewed at head But this is a HOLD on one new defect that undoes a guarantee an earlier commit in this same PR established. 1. BLOCKING:
Reproduced on your head with a This is exactly the failure mode your own commit It is invisible to the suite because 2. 3. 4. 5. Three silent scope shortfalls vs #2059 (pitfall 19 — state deferrals explicitly rather than dropping them):
Credit where it is due on SSRF, because this is the strongest part of the PR: Bot state, so you do not chase noise: all checks including both test matrices genuinely ran and passed on this fork PR. Kilo's last summary reviewed an earlier increment and explicitly scoped itself out of Fold 1 is the gate. |
|
Folded all review findings at c8f6416: BLOCKING fix: routes/library.py — Non-blocking fixes:
51/51 targeted tests pass. |
| "for %s (content-type=%s)", | ||
| source_url, resp_content_type, | ||
| ) | ||
| return artifacts |
There was a problem hiding this comment.
🔥 The Roast: Non-text responses (PDFs, images, videos, binaries) get a polite logger.info and an empty artifact list — then the pipeline cheerfully marks the item ready. The user sees a green checkmark, the collections indexer gets nothing, and nobody knows the URL pointed to a 50MB ZIP file instead of an article. "Successfully processed! (Just kidding, we skipped it.)"
🩹 The Fix: Either mark the item error with a clear reason (non-text content-type: application/pdf), or introduce a distinct terminal status like skipped/unsupported. At minimum, don't let ready mean "we looked at it and shrugged."
📏 Severity: warning
| try: | ||
| p.kill() | ||
| except ProcessLookupError: | ||
| pass |
There was a problem hiding this comment.
🔥 The Roast: _cleanup_procs only catches ProcessLookupError. If p.kill() raises PermissionError (container restrictions) or OSError (weird kernel state), the exception escapes the finally block and masks the original CancelledError/TimeoutError. The root cause gets lost in the noise.
🩹 The Fix:
| pass | |
| for p in _procs: | |
| if p.returncode is None: | |
| try: | |
| p.kill() | |
| except Exception: | |
| pass |
📏 Severity: nitpick
| assert "transcript" in artifact_kinds | ||
|
|
||
|
|
||
| class TestWebProcessor: |
There was a problem hiding this comment.
🔥 The Roast: The WebProcessor just got three shiny new security features — manual redirect validation (SSRF per hop), 10 MB streaming size cap, and content-type gating — and the test suite doesn't exercise a single one. test_process_web_url still mocks a happy-path 200 OK text/html response. No test for redirect chains, no test for size-limit ValueError, no test for image/png early return, no test for SsrfBlockedError on redirect loops. These are the exact paths that bite you in production.
🩹 The Fix: Add focused tests for each new guard:
test_web_redirect_chain_validated_per_hoptest_web_size_cap_raises_errortest_web_non_text_content_type_returns_empty(or errors, see library_pipeline.py:452)test_web_too_many_redirects_raises_ssrf_blocked
📏 Severity: suggestion
| assert len(artifacts) == 0 | ||
|
|
||
|
|
||
| class TestYouTubeProcessor: |
There was a problem hiding this comment.
🔥 The Roast: YouTubeProcessor now wraps the whole fetch() call in asyncio.wait_for(..., timeout=120) and the fetcher tracks subprocesses to kill them on cancellation — but there's no test verifying the timeout fires, no test verifying zombie yt-dlp processes get reaped on CancelledError, and no test for the 120s boundary. The happy path is covered; the failure modes that motivated the code aren't.
🩹 The Fix: Add tests using pytest.mark.asyncio with a patched fetch that await asyncio.sleep(200) to trigger the timeout, and assert the item status becomes error. Also verify _cleanup_procs is called (mock p.kill).
📏 Severity: suggestion
| _MAX_WEB_BYTES = 10 * 1024 * 1024 # 10 MB | ||
|
|
||
| current_url = source_url | ||
| resp_status_code: int = 0 |
There was a problem hiding this comment.
🔥 The Roast: resp_status_code and resp_content_type are captured inside the redirect loop but declared as resp_status_code: int = 0 and resp_content_type: str = "" up top. The names imply they belong to the final response, but they're actually mutated inside the loop. A future maintainer will wonder why they're initialized to dummy values instead of being assigned once after the loop breaks. It works, but it smells like a leftover from when the code lived outside the loop.
🩹 The Fix: Move the declarations inside the non-redirect block where they're actually set, or assign them once after the loop using the final resp (which is still in scope if you restructure slightly). Current pattern works but reads like a half-refactor.
📏 Severity: nitpick
…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.
|
Held after a deep review. The SSRF guard is genuinely solid (per-hop, pre-DNS + post-resolution, handles encoded/IPv6-mapped/CGNAT), no eval/pickle/shell, and the HTMX card escapes attacker fields. But two advertised protections do not work and the PR silently reverts prior hardening. Verified each against the code before flagging. MEDIUM - the 10MB web size cap is ineffective (verified). MEDIUM - YouTube fetch has no timeout. youtube.py has three MEDIUM (regression) - reprocess lost its concurrency guard and idempotent cleanup (verified: LOW: the unauth-endpoints test ( Tests are happy-path only - WebProcessor tests patch Fix the two ineffective caps and restore the reprocess guard + its tests, then I will re-review. The SSRF work itself is good and I do not want it lost in the churn. |
| logger = logging.getLogger(__name__) | ||
|
|
||
| # Text artifact kinds that should be indexed into collections | ||
| _TEXT_ARTIFACT_KINDS = frozenset({"text", "transcript", "description", "ocr"}) |
There was a problem hiding this comment.
WARNING: TEXT_ARTIFACT_KINDS excludes "chapters"
YouTubeProcessor writes chapter text as a chapters artifact, but handoff_to_collections only indexes kinds matching {"text", "transcript", "description", "ocr"}. This silently drops YouTube chapter text from the collections index even though it is structured, searchable text content. Add "chapters" to the frozenset so chapter titles are indexed alongside transcripts.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
Deep review done. Verdict: please rebase and re-land rather than fix-forward. The P2 feature code is mostly sound; the problem is the branch, not the feature. BLOCKER 1: this branch reverts merged P1 hardening on dev. Merge base is
CI green is not evidence here. Because dev has not touched these files, the merge result IS this branch, so the 47 passing tests are this branch's own weakened suite. The deleted auth and reprocess tests cannot fail because they no longer exist. BLOCKER 2: collections and grants are bypassed.
HIGH: the 10 MB cap does not work, and it is an OOM vector. In Exploit: ingest a URL whose server streams a multi-gigabyte Fix: use MEDIUM: the 2026-07-27 force-push dropped earlier review fixes. The force-push at 07:46:47 replaced four commits with six rebased ones, and lost MEDIUM: no total deadline. MEDIUM: duplicate UI. This re-adds Good news, verified rather than assumed:
Two live bot findings worth folding: a redirect ending at a PDF or image yields a green "ready" item with zero content, and Recommended path: rebase |
…r text is indexed into collections
YouTubeProcessor writes chapter text as a 'chapters' artifact kind, but
handoff_to_collections only indexed kinds matching {text, transcript,
description, ocr}. Adding 'chapters' ensures YouTube chapter artifacts
are included in the taosmd collection index.
Fixes jaylfc#2068
|
Force-pushed fix for the Kilo WARNING on
|
|
Re-reviewed at head
The silent revert is confirmed, and nothing will stop it. Sharpest single item: dev asserts The 10MB cap, measured rather than argued. 130MB peak for a 10MB cap, scaling linearly with an attacker-controlled body. On a 4GB Pi that is a remote OOM-kill of the controller from one ingest URL. Fix: The content-type gate is not merely post-download, it does not exist at head. On the green checks. All 17 pass. I ran What I want, and I think this is less work than it sounds: rebase To be clear about what is good here: the SSRF work is the strongest part of this PR and I verified it is intact at head - per-hop |
…content-type gate, timeout guards (#2177) Rebase feat/library-p2 onto origin/dev (c5b1a6f) keeping ONLY P2 delta: - YouTubeProcessor: metadata, thumbnail, transcript, chapters via yt-dlp - WebProcessor: HTML fetch, readability extraction, SSRF-guarded redirects - _extract_readable_text() helper (readability-lxml with tag-stripping fallback) - url:youtube + url:web entries in _PROCESSORS - run_pipeline reference-artifact removal (URL items now get processed) - _store_init_lock: asyncio.Lock on lazy LibraryStore init (TOCTOU fix) - 'chapters' added to _TEXT_ARTIFACT_KINDS Fixes from jaylfc review of #2068: - Streaming read via client.stream() — OOM-safe, cap enforced before buffering - Content-type gate: non-text/* responses raise ValueError (→ error status) - 60s wall-clock deadline on WebProcessor fetch - _cleanup_procs() in youtube.py: kill tracked subprocesses on timeout - 120s asyncio.wait_for on YouTubeProcessor fetch with cleanup on TimeoutError Preserves all P1 hardening from dev: - try_update_item_status CAS (library_store.py untouched) - reprocess 409 guard + artifact cleanup + rollback-to-ready - taosmd HTTP collections flow (memory_url, admin-token, idempotent collection_id) - logger.warning OSError handlers (not bare pass) - path='' on metadata artifacts (unlink guard) - test_library_page_gone (404), test_unauth_library_endpoints, test_reprocess_idempotent, test_reprocess_while_processing_returns_409 Tests: 54/54 pass (42 existing + 12 new: 3 YouTube, 4 web happy-path, 2 SSRF block, 1 redirect-hop validation, 1 size cap, 1 content-type gate) Co-authored-by: Hogne <227774406+hognek@users.noreply.github.com>
|
Superseded by #2177, which merged at 00:04 as #2177 is the rebase I asked for here and it addressed every blocking finding: the silent revert of merged P1 hardening is gone (verified by symbol-diff against Closing this rather than leaving it conflicted, so nobody rebases the wrong branch. The SSRF chain you built here survived into #2177 intact, which was always the reason this was worth rebasing rather than abandoning. |
2 participants
Summary
Builds on PR #2062 (P1 Library core) — adds YouTube cheap-tier ingest and generic web-page ingestor as P2 of epic #2057.
Changes
YouTubeProcessor— handlesurl:youtubeitems. Fetches metadata, thumbnail, transcript, and chapters viaknowledge_fetchers.youtube(yt-dlp). Cheap-tier only: no video download, just text artifacts for collection indexing. Per design doc sections 3–4.WebProcessor— handlesurl:webitems. Fetches HTML (SSRF-guarded viavalidate_url_or_raise), extracts readable text using readability-lxml (falls back to simple tag-stripping). Auto-titles from\<title\>tag. Produces text artifacts for collection indexing._PROCESSORSdict sorun_pipelinedispatches to them._extract_readable_text()helper — shared readability extraction with readability-lxml priority and tag-stripping fallback.Files changed
tinyagentos/library_pipeline.py— +258 lines (YouTubeProcessor, WebProcessor,_extract_readable_text, registry entries)tests/test_library.py— +290 lines (8 new tests: 4 YouTube, 4 web; 47/47 all pass)Design doc
docs/design/library-app.mdsections 3–4 (cheap tier + YouTube reference flow, steps 1-4)Testing
All 47 library tests pass. No new imports break existing tests.
Summary by Gitar
tinyagentos/library_pipeline.pywith multi-kind processor support forfile,text,pdf,image,url:youtube, andurl:web.LibraryStoreintinyagentos/library_store.pyfor tracking ingested items, artifacts, and async processing jobs.tinyagentos/routes/library.pywith support for file uploads and URL ingest.tinyagentos/library_collections.pyto index text artifacts intotaosmdcollections for agent queryability.tinyagentos/templates/library.htmlwith drag-and-drop ingestion, progress indicators, and real-time HTMX-based list updates.tests/test_library.pycovering kind detection, store operations, processor logic, and route API endpoints.This will update automatically on new commits.
Summary by CodeRabbit