fix(library): address CodeRabbit CRITICAL/MAJOR findings from PR #2068#2085
Closed
hognek wants to merge 6 commits into
Closed
fix(library): address CodeRabbit CRITICAL/MAJOR findings from PR #2068#2085hognek wants to merge 6 commits into
hognek wants to merge 6 commits into
Conversation
…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.
…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.
…patterns, test assertions
…#2068 CRITICAL: fix httpx stream context bug — resp used after async with exits in WebProcessor redirect loop. Body consumption moved inside stream context. MAJOR: reprocess failure-safety — roll back to error (not ready) after artifact deletion to avoid data-less items stuck in ready state. MAJOR: serialise reprocess against collections handoff — CAS-flip back to processing in _ingest_task so reprocess sees busy state during handoff. MAJOR: add asyncio.wait_for(120s) timeout on yt-dlp fetch in YouTubeProcessor. MAJOR: validate collection-link POST response — raise_for_status() so HTTP 401/404/500 are caught instead of silently clearing retryable state. MINOR: replace flaky asyncio.sleep(0.5) with bounded status polling in reprocess idempotent/409 tests. MINOR: remove redundant local imports of AsyncMock/MagicMock/patch in test_handoff_with_qmd; promote AsyncMock to module level. MINOR: fix docstring/comment from 6 to 5 endpoints in test_unauth_library. MINOR: prefer next() over list comprehension indexing for thumbnail lookup. 50/50 tests pass.
hognek
marked this pull request as ready for review
July 21, 2026 18:14
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? |
📝 WalkthroughWalkthroughLibrary ingestion now supports YouTube and web URL processors, validates collection-link responses, serializes store initialization, claims ingestion atomically, and records explicit ready/error states. Tests add async dependency mocks and poll processing state transitions deterministically. ChangesLibrary ingest flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant LibraryRoute
participant run_pipeline
participant YouTubeProcessor
participant WebProcessor
participant LibraryStore
participant CollectionHandoff
LibraryRoute->>LibraryStore: claim item as processing
LibraryRoute->>run_pipeline: process item
run_pipeline->>YouTubeProcessor: route url:youtube
YouTubeProcessor->>LibraryStore: save video artifacts and metadata
run_pipeline->>WebProcessor: route url:web
WebProcessor->>LibraryStore: save extracted text and metadata
LibraryRoute->>CollectionHandoff: hand off indexed collection
CollectionHandoff-->>LibraryRoute: link response
LibraryRoute->>LibraryStore: set ready or error 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/routes/library.py`:
- Around line 181-188: Prevent the collections handoff from reacquiring items
whose processing already failed: update the status transition in the handoff
logic to exclude the "error" state, or propagate failures from run_pipeline so
the handoff is not executed after an error. Preserve the existing CAS behavior
for pending and processing items and ensure failed ingest items cannot be
overwritten as ready.
🪄 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: 19984ca4-b02c-496a-a2b7-57c06ee25229
📒 Files selected for processing (4)
tests/test_library.pytinyagentos/library_collections.pytinyagentos/library_pipeline.pytinyagentos/routes/library.py
Comment on lines
+181
to
+188
| # Re-acquire processing state so reprocess sees us as busy during the | ||
| # collections handoff window. If we lose the CAS race, another pipeline | ||
| # already claimed the item — abort cleanly (it will do its own handoff). | ||
| if not await store.try_update_item_status( | ||
| item_id, "processing", if_not_in=("pending", "processing") | ||
| ): | ||
| return | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Show run_pipeline to verify its except block does not re-raise.
fd -t f 'library_pipeline.py' --exec ast-grep run --pattern $'async def run_pipeline($$$):\n $$$' --lang python {}
rg -nP -A6 'except Exception' -g '**/library_pipeline.py'Repository: jaylfc/taOS
Length of output: 149
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files and inspect the surrounding code.
fd -a 'library.py|library_pipeline.py|store\.py|.*store.*\.py' .
printf '\n--- tinyagentos/routes/library.py outline ---\n'
ast-grep outline tinyagentos/routes/library.py --view expanded || true
printf '\n--- tinyagentos/routes/library.py lines 140-240 ---\n'
sed -n '140,240p' tinyagentos/routes/library.py
printf '\n--- search for run_pipeline definitions/usages ---\n'
rg -n "async def run_pipeline|run_pipeline\(" .
printf '\n--- files with try_update_item_status / get_item / update_item_status ---\n'
rg -n "try_update_item_status|get_item\(|update_item_status\(" tinyagentos .Repository: jaylfc/taOS
Length of output: 22569
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- tinyagentos/library_pipeline.py lines 632-720 ---\n'
sed -n '632,720p' tinyagentos/library_pipeline.py
printf '\n--- tinyagentos/library_store.py lines 100-220 ---\n'
sed -n '100,220p' tinyagentos/library_store.py
printf '\n--- tinyagentos/routes/library.py lines 168-222 ---\n'
sed -n '168,222p' tinyagentos/routes/library.pyRepository: jaylfc/taOS
Length of output: 10413
Failed ingest items can be resurrected to ready. run_pipeline records "error" and returns, so this path still reacquires the item from error, runs the collections step, and can overwrite the failure with ready. Skip the handoff when the item is already in an error state, or have run_pipeline propagate failures.
🤖 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 181 - 188, Prevent the
collections handoff from reacquiring items whose processing already failed:
update the status transition in the handoff logic to exclude the "error" state,
or propagate failures from run_pipeline so the handoff is not executed after an
error. Preserve the existing CAS behavior for pending and processing items and
ensure failed ingest items cannot be overwritten as ready.
Owner
|
Closing this as a duplicate. Superseded by #2068, which is the canonical Library P2 PR. Both PRs now point at the identical commit ( Worth naming the pattern rather than just closing, since it is the third time this shape has come up: a finding on an open PR gets folded into that PR, not into a sibling. A second PR for the same slice splits the review history, doubles the CI cost, and makes it ambiguous which one a reviewer should read. The taOS development skill covers this under "one PR per slice" and pitfall 13 (a fix and its test belong in one PR). Nothing lost by closing this one: #2068 carries the same commits plus the earlier review thread. Continue there. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes CRITICAL and MAJOR findings from CodeRabbit review of PR #2068 (review rounds 2026-07-21T16:06Z and 2026-07-21T17:04Z).
CRITICAL
respfromclient.stream()was accessed after itsasync withcontext exited when the redirect loop broke. Movedraise_for_status()+ body consumption inside the stream context.MAJOR
error(notready) — rolling back to ready leaves a data-less item._ingest_tasknow CAS-flips back toprocessingbefore the collections handoff, so reprocess sees busy state during the handoff window. Handoff now explicitly setsreadyon success,erroron failure.fetch()withasyncio.wait_for(timeout=120)to prevent hung yt-dlp subprocesses from blocking the pipeline indefinitely.raise_for_status()on the link POST response so HTTP 401/404/500 are caught instead of silently clearing retryable state.MINOR
asyncio.sleep(0.5)with bounded status polling in reprocess testsnext()over list comprehension indexing for thumbnail lookupSkipped
updated_title == "Test Video"exists.Testing
All 50 library tests pass (
pytest tests/test_library.py -n auto).Fixes findings from reviews on #2068.
Summary by Gitar
YouTubeProcessorto ingest metadata, transcripts, and chapters usingyt-dlpwith a 120s timeout.WebProcessorto extract readable content from HTML pages with SSRF protection and response size capping.LibraryStorelazy initialization withasyncio.Lockto prevent race conditions during concurrent access._ingest_taskto CAS-flip status toprocessingbefore handoff to ensure reprocess ignores in-flight tasks.asyncio.sleepcalls with robust status polling intest_library.py.This will update automatically on new commits.
Summary by CodeRabbit
New Features
Bug Fixes