fix(comments): address code-review BLOCKER + 4 IMPORTANT issues
Some checks failed
CI / lint (push) Successful in 32s
Deploy / notebooks (push) Has been skipped
Deploy / zotero (push) Has been skipped
Deploy / api (push) Has been skipped
Deploy / mc (push) Has been skipped
Infra CI / notebooks (push) Failing after 14s
Package Supply Chain / pkg-supply-chain (push) Failing after 34s
Deploy / report (push) Successful in 28s
Deploy / docs (push) Has been skipped
Infra CI / zotero (push) Successful in 12s
Infra CI / docs (push) Successful in 1m16s
Infra CI / api (push) Successful in 23s
Infra CI / mc (push) Successful in 12s
CI / test (push) Has been cancelled

From the end-of-impl review:

1. BLOCKER: SQLite cross-thread error in stack comments extract --bib.
   _bib_lookup_factory was sharing the bib.connect() connection across
   walker worker threads — sqlite3 defaults to check_same_thread=True
   so every worker would have raised ProgrammingError. Tests didn't
   catch it because all CLI tests pass --no-bib. Fix: open a fresh
   read-only connection with check_same_thread=False directly via
   conf.path("db.bib"); SQLite handles concurrent reads safely.

2. view._iter_index_rows now catches yaml.YAMLError too, so one
   malformed combined.md doesn't crash `stack comments stats`.

3. Image attachments (.jpg/.png/.tif/.tiff/.gif/.bmp) now classify as
   ocr_needed instead of being silently dropped as unsupported. This
   gives the phase-2 OCR command a clean candidate set to query.
   Real CMS comments include scanned-letter images.

4. view.register() defaults to rebuild=True so downstream consumers
   (#254/#255) always see fresh data. Pass rebuild=False to skip.

5. extract_comment writes via tmp file + os.replace() so concurrent
   walkers / restarts never see a half-written combined.md.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
kert
2026-04-23 16:09:47 -04:00
parent da831c04ff
commit 7281012c76
5 changed files with 49 additions and 9 deletions

View File

@@ -21,14 +21,21 @@ _DEFAULT_ROOT = Path(".state/comments")
def _bib_lookup_factory(use_bib: bool):
"""Return a callable comment_id -> inline body. Empty if --no-bib."""
"""Return a callable ``comment_id -> inline body``. Empty if --no-bib.
Opens its own sqlite connection with ``check_same_thread=False`` so
walker worker threads can call it directly. Read-only path; SQLite's
default thread-safe mode handles the concurrent reads.
"""
if not use_bib:
return lambda _cid: ""
from bib import connect
import sqlite3
store = connect()
con = store._con() # noqa: SLF001
from conf import path
db = str(path("db.bib"))
con = sqlite3.connect(db, check_same_thread=False)
def lookup(comment_id: str) -> str:
row = con.execute(

View File

@@ -7,6 +7,7 @@ sections). Resumable: existing combined.md is a no-op unless force=True.
from __future__ import annotations
import os
from datetime import datetime, timezone
from pathlib import Path
from typing import Any
@@ -62,7 +63,11 @@ def extract_comment(
"attachments": attachments_meta,
}
out.write_text(_emit(frontmatter, body_text))
# Atomic write: tmp file + rename, so concurrent walkers / restarts
# never see a half-written combined.md.
tmp = out.with_suffix(out.suffix + ".tmp")
tmp.write_text(_emit(frontmatter, body_text))
os.replace(tmp, out)
return out

View File

@@ -26,6 +26,9 @@ class ExtractResult:
chars: int
_IMAGE_SUFFIXES = (".jpg", ".jpeg", ".png", ".tif", ".tiff", ".gif", ".bmp")
def extract_attachment(path: Path) -> ExtractResult:
"""Extract text from one attachment. See module docstring for status values."""
suffix = path.suffix.lower()
@@ -35,6 +38,10 @@ def extract_attachment(path: Path) -> ExtractResult:
return _extract_docx(path)
if suffix in (".txt", ".html", ".htm"):
return _extract_text_like(path, suffix)
if suffix in _IMAGE_SUFFIXES:
# Real CMS comments include scanned-letter images; route to
# phase-2 OCR queue, don't drop them as "unsupported".
return ExtractResult(text="", status="ocr_needed", chars=0)
return ExtractResult(text="", status="unsupported", chars=0)

View File

@@ -12,6 +12,7 @@ import csv
from pathlib import Path
import duckdb
import yaml
from rex.comments.combine import parse_combined
@@ -40,10 +41,20 @@ def rebuild_index(root: Path) -> Path:
return out
def register(con: duckdb.DuckDBPyConnection, *, root: Path) -> None:
"""Create the ``comments_index`` view in *con* over *root*/_index.csv."""
def register(
con: duckdb.DuckDBPyConnection,
*,
root: Path,
rebuild: bool = True,
) -> None:
"""Create the ``comments_index`` view in *con* over *root*/_index.csv.
Rebuilds the index by default — downstream queries should see fresh
data. Pass ``rebuild=False`` to skip the walk if you've already
rebuilt this session and the disk hasn't changed.
"""
csv_path = root / _INDEX
if not csv_path.is_file():
if rebuild or not csv_path.is_file():
rebuild_index(root)
con.execute(
f"CREATE OR REPLACE VIEW {_VIEW} AS "
@@ -69,7 +80,7 @@ def _iter_index_rows(root: Path):
continue
try:
fm, _body = parse_combined(md.read_text())
except (ValueError, OSError):
except (ValueError, OSError, yaml.YAMLError):
continue
atts = fm.get("attachments") or []
yield (

View File

@@ -33,3 +33,13 @@ def test_extract_unknown_extension(tmp_path: Path):
result = extract_attachment(p)
assert result.status == "unsupported"
assert result.chars == 0
def test_extract_image_marked_ocr_needed(tmp_path: Path):
"""Image attachments (scanned letters) route to phase-2 OCR queue."""
for ext in (".jpg", ".jpeg", ".png", ".tif", ".tiff", ".gif", ".bmp"):
p = tmp_path / f"scan{ext}"
p.write_bytes(b"fake image bytes")
result = extract_attachment(p)
assert result.status == "ocr_needed", f"{ext}: {result.status}"
assert result.chars == 0