-
-
Notifications
You must be signed in to change notification settings - Fork 38
[lib-audit] Q2-4 ingest and library nits (100 MB str copy, title unescape, JPEG modes, single-json, timeouts, chaining) #2928
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| ### Fixed | ||
|
|
||
| - TextProcessor now streams file reads instead of loading the entire file into a single `str`, eliminating the 100 MB `str` copy on large text files. | ||
| - Article title extraction in `knowledge_ingest._download_article` now unescapes HTML entities (e.g. `&` becomes `&`), matching the behaviour already present in `library_pipeline.WebProcessor`. | ||
| - ImageProcessor JPEG thumbnail conversion now handles `LA`, `PA`, `I;16` and other non-RGB/L/CMYK Pillow modes instead of raising on them. | ||
| - `x.py` `create_watch` now chains the `sqlite3.IntegrityError` via `raise ... from e`. | ||
| - `verify_registry_token` now raises `ValueError` with a clear message when the JWT payload is not a JSON object (dict). |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -154,21 +154,32 @@ async def process(self, item: dict) -> list[dict]: | |
| return artifacts | ||
|
|
||
| try: | ||
| text = p.read_text(encoding="utf-8", errors="replace") | ||
| char_count = 0 | ||
| line_count = 1 | ||
| preview = "" | ||
| text_dir = self.storage_dir / "text" | ||
| text_dir.mkdir(parents=True, exist_ok=True) | ||
| text_path = text_dir / f"{item_id}.txt" | ||
| with open(p, "r", encoding="utf-8", errors="replace") as src: | ||
| with open(text_path, "w", encoding="utf-8") as dst: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WARNING: TextProcessor silently swallows filesystem errors during streaming write In the new streaming implementation, Reply with |
||
| while True: | ||
| chunk = src.read(8192) | ||
| if not chunk: | ||
| break | ||
| dst.write(chunk) | ||
| char_count += len(chunk) | ||
| line_count += chunk.count("\n") | ||
| if len(preview) < 200: | ||
| preview += chunk | ||
| preview = preview[:200] | ||
| except Exception: | ||
| logger.warning("Text processor: could not read %s", storage_path, | ||
| exc_info=True) | ||
| return artifacts | ||
|
|
||
| # Write extracted text as an artifact | ||
| text_dir = self.storage_dir / "text" | ||
| text_dir.mkdir(parents=True, exist_ok=True) | ||
| text_path = text_dir / f"{item_id}.txt" | ||
| text_path.write_text(text, encoding="utf-8") | ||
|
|
||
| text_meta = { | ||
| "char_count": len(text), | ||
| "line_count": text.count("\n") + 1, | ||
| "char_count": char_count, | ||
| "line_count": line_count, | ||
| "source_url": item.get("source_url", ""), | ||
| "processed_at": time.time(), | ||
| "processor": "TextProcessor/v1", | ||
|
|
@@ -179,14 +190,13 @@ async def process(self, item: dict) -> list[dict]: | |
| artifacts.append({"kind": "text", "path": str(text_path), "meta": text_meta}) | ||
|
|
||
| # Store a preview (first 200 chars) | ||
| preview = text[:200] | ||
| meta = json.loads(item.get("meta_json", "{}")) | ||
| meta["preview"] = preview | ||
| await self.store.update_item(item_id, meta_json=meta) | ||
|
|
||
| # Auto-title from content if no title | ||
| if not item.get("title"): | ||
| title = text.strip().split("\n", 1)[0][:100] | ||
| title = preview.strip().split("\n", 1)[0][:100] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Continue title detection after the preview limit. If a file starts with 200 or more whitespace characters, 🤖 Prompt for AI Agents |
||
| if title: | ||
| await self.store.update_item(item_id, title=title) | ||
|
|
||
|
|
@@ -295,8 +305,10 @@ async def process(self, item: dict) -> list[dict]: | |
| thumb_path = thumb_dir / f"{item_id}_thumb.jpg" | ||
|
|
||
| img.thumbnail((320, 320)) | ||
| # Convert to RGB if needed (e.g. RGBA/PNG → JPEG) | ||
| if img.mode in ("RGBA", "P"): | ||
| # Convert to RGB if needed (e.g. RGBA/PNG → JPEG). | ||
| # JPEG supports only "L", "RGB", "CMYK"; all other modes | ||
| # (including LA, PA, I;16) must be converted first. | ||
| if img.mode not in ("RGB", "L", "CMYK"): | ||
| img = img.convert("RGB") | ||
| img.save(thumb_path, "JPEG", quality=75) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Resolve the reported RUF059 warnings.
The test uses only
title. Rename the other unpacked values with a leading underscore, such as_content,_author, and_metadata.🧰 Tools
🪛 Ruff (0.16.3)
[warning] 754-754: Unpacked variable
contentis never usedPrefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 754-754: Unpacked variable
authoris never usedPrefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 754-754: Unpacked variable
metadatais never usedPrefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
Source: Linters/SAST tools