fix(bib): import-zotero — honest docstrings, missing-file warning, CLI test (refs #687)

src/bib/zotero_import.py

@@ -4,7 +4,8 @@
 access to the Zotero SQLite database (``conf.connect.zotero()``), so
 material the user already curated in Zotero — the AMA CPT manuals, for
 instance — can be cited by ``item_key`` like everything else in bib,
-files included.
+files included, and indexed into the chat corpus like any other
+document (see ``llm.source.iter_corpus_refs``).
 
 Usage::
 
@@ -12,11 +13,12 @@ Usage::
 
     zcon = zotero()
     books = list_collection(zcon, "AMA Coding Publications", with_files=True)
-    stats = import_books(bib(), books, tags=["source:ama", "llm:skip"])
+    stats = import_books(bib(), books, tags=["source:ama", "module:coding"])
 """
 
 from __future__ import annotations
 
+import logging
 import sqlite3
 from dataclasses import dataclass
 from pathlib import Path
@@ -25,6 +27,8 @@ from typing import TYPE_CHECKING, Sequence
 if TYPE_CHECKING:
     from bib.store import Store
 
+log = logging.getLogger(__name__)
+
 _FIELD_NAMES = ("title", "url", "date", "publisher")
 
 
@@ -141,11 +145,22 @@ def import_books(
     Returns a summary dict: ``created``/``updated``/``unchanged``
     counts items by :meth:`Store.upsert_status` outcome, ``attached``
     counts newly-copied files (a re-run's dedup'd attachments don't
-    count), and ``items`` lists one record per book for reporting.
+    count), ``missing`` counts attachment paths that don't exist on
+    disk (logged and skipped, never silently dropped), and ``items``
+    lists one record per book for reporting — each with its own
+    ``attached``, ``missing``, and ``missing_paths`` (the paths that
+    were missing, for a caller to surface them).
     """
     from bib.item import Source
 
-    stats = {"created": 0, "updated": 0, "unchanged": 0, "attached": 0, "items": []}
+    stats = {
+        "created": 0,
+        "updated": 0,
+        "unchanged": 0,
+        "attached": 0,
+        "missing": 0,
+        "items": [],
+    }
 
     for book in books:
         url = book.url or f"zotero://select/library/items/{book.key}"
@@ -160,7 +175,7 @@ def import_books(
             )
             status = "unchanged" if existing else "created"
             stats[status] += 1
-            n_new = 0
+            n_new, n_missing, missing_paths = 0, 0, []
             if copy_files:
                 have = (
                     {
@@ -176,8 +191,15 @@ def import_books(
                     if existing
                     else set()
                 )
-                n_new = sum(1 for p in book.attachments if p.name not in have)
+                for p in book.attachments:
+                    if not p.exists():
+                        log.warning("missing on disk: %s", p)
+                        missing_paths.append(str(p))
+                        n_missing += 1
+                    elif p.name not in have:
+                        n_new += 1
             stats["attached"] += n_new
+            stats["missing"] += n_missing
             stats["items"].append(
                 {
                     "key": existing[0] if existing else "",
@@ -185,6 +207,8 @@ def import_books(
                     "title": book.title,
                     "status": status,
                     "attached": n_new,
+                    "missing": n_missing,
+                    "missing_paths": missing_paths,
                 }
             )
             continue
@@ -199,10 +223,13 @@ def import_books(
         key, status = store.upsert_status(item, tags=item_tags)
         stats[status] += 1
 
-        n_new = 0
+        n_new, n_missing, missing_paths = 0, 0, []
         if copy_files:
             for att_path in book.attachments:
                 if not att_path.exists():
+                    log.warning("missing on disk: %s", att_path)
+                    missing_paths.append(str(att_path))
+                    n_missing += 1
                     continue
                 dup = (
                     store._con()
@@ -218,6 +245,7 @@ def import_books(
                 if dup is None:
                     n_new += 1
         stats["attached"] += n_new
+        stats["missing"] += n_missing
         stats["items"].append(
             {
                 "key": key,
@@ -225,6 +253,8 @@ def import_books(
                 "title": book.title,
                 "status": status,
                 "attached": n_new,
+                "missing": n_missing,
+                "missing_paths": missing_paths,
             }
         )
 

src/cli/bib.py

@@ -547,10 +547,13 @@ def import_zotero(
 
     The reverse of ``sync-zotero``: reads the Zotero SQLite database
     read-only and upserts each book as a bib ``Source`` item (dedup by
-    URL), copying its attachment file(s) into bib storage. Tag with
+    URL), copying its attachment file(s) into bib storage — an ordinary
-    ``llm:skip`` to keep material out of the corpus indexer (see
+    corpus document like any other, indexed like everything else in
-    ``llm.source.iter_corpus_refs``) — the escape hatch for
+    bib. ``llm:skip`` is available as a generic opt-out for material
-    copyrighted text imported for citation/metadata only.
+    that should not be embedded (see ``llm.source.iter_corpus_refs``)
+    — e.g. a scratch export or a duplicate scan — but it's never added
+    automatically here; pass ``--tag llm:skip`` explicitly if it
+    applies.
     """
     from bib import connect
     from bib.zotero_import import import_books, list_collection
@@ -578,10 +581,13 @@ def import_zotero(
             f'{prefix}{rec["status"]} "{rec["title"][:60]}" '
             f"attachments={rec['attached']}"
         )
+        for missing_path in rec.get("missing_paths", []):
+            typer.echo(f"! missing on disk: {missing_path}", err=True)
 
     typer.echo(
         f"{prefix}created={stats['created']} {prefix}updated={stats['updated']} "
-        f"{prefix}unchanged={stats['unchanged']} {prefix}attached={stats['attached']}"
+        f"{prefix}unchanged={stats['unchanged']} {prefix}attached={stats['attached']} "
+        f"missing={stats['missing']}"
     )
 
 

src/llm/source.py

@@ -499,9 +499,10 @@ def iter_corpus_refs(
     updated_at + the bib attachment file stats; the Zotero fallback is
     only consulted by ``load()``.
 
-    Items tagged ``llm:skip`` are excluded — the escape hatch for
+    Items tagged ``llm:skip`` are excluded — a generic opt-out for
-    material that must never enter the indexer (e.g. copyrighted CPT
+    material that should not be embedded (e.g. a scratch export, or a
-    editions imported for citation only, not for RAG text)."""
+    duplicate scan of something already in the corpus). It's never
+    applied automatically; a caller opts an item in by tagging it."""
     sql = (
         "SELECT i.key, COALESCE(i.updated_at,'') AS updated_at FROM items i "
         "WHERE i.id NOT IN (SELECT item_id FROM item_tags WHERE tag_id IN "

tests/bib/test_zotero_import.py

@@ -223,3 +223,32 @@ class TestImportBooks:
         assert stats["unchanged"] == 1
         assert stats["attached"] == 0
         assert stats["items"][0]["key"]  # bib key resolved from the existing url
+
+    def test_missing_attachment_file_counts_and_skips(self, store, storage_dir):
+        ghost_path = storage_dir / "GHOSTKEY" / "missing.pdf"
+        book = ZoteroBook(
+            key="GHOSTBK1",
+            title="Ghost Book",
+            url="https://ama.example/ghost",
+            year="2020",
+            publisher="American Medical Association",
+            attachments=(ghost_path,),
+        )
+        stats = import_books(store, [book], tags=["source:ama"])
+
+        assert stats["missing"] == 1
+        assert stats["attached"] == 0
+        (rec,) = stats["items"]
+        assert rec["missing"] == 1
+        assert rec["missing_paths"] == [str(ghost_path)]
+
+        rows = (
+            store._con()
+            .execute(
+                "SELECT filename FROM attachments a "
+                "JOIN items i ON i.id = a.item_id WHERE i.key = ?",
+                (rec["key"],),
+            )
+            .fetchall()
+        )
+        assert rows == []  # no attachment row for the missing file

tests/cli/test_bib_deep.py

@@ -55,6 +55,176 @@ class TestDiscoverPfsRules:
         assert result.exit_code == 0
 
 
+class TestImportZotero:
+    @patch("bib.zotero_import.import_books")
+    @patch("bib.zotero_import.list_collection")
+    @patch("conf.connect.zotero", return_value=MagicMock())
+    @patch("bib.connect", return_value=MagicMock())
+    def test_dry_run_prints_would_create_and_writes_nothing(
+        self, mc_connect, mc_zotero, mc_list, mc_import
+    ):
+        from bib.zotero_import import ZoteroBook
+
+        book = ZoteroBook(
+            key="ZKEY1",
+            title="CPT 2018",
+            url="https://x",
+            year="2017",
+            publisher="AMA",
+            attachments=(),
+        )
+        mc_list.return_value = [book]
+        mc_import.return_value = {
+            "created": 1,
+            "updated": 0,
+            "unchanged": 0,
+            "attached": 0,
+            "missing": 0,
+            "items": [
+                {
+                    "key": "",
+                    "zotero_key": "ZKEY1",
+                    "title": "CPT 2018",
+                    "status": "created",
+                    "attached": 0,
+                    "missing": 0,
+                    "missing_paths": [],
+                }
+            ],
+        }
+
+        result = runner.invoke(
+            app,
+            [
+                "import-zotero",
+                "--collection",
+                "AMA Coding Publications",
+                "--dry-run",
+            ],
+        )
+
+        assert result.exit_code == 0
+        assert "would-created" in result.stdout
+        assert "CPT 2018" in result.stdout
+        # dry_run=True is forwarded to import_books, which is the sole
+        # place writes happen — the CLI itself performs none.
+        _, kwargs = mc_import.call_args
+        assert kwargs["dry_run"] is True
+        assert kwargs["tags"] == []
+
+    @patch("bib.zotero_import.import_books")
+    @patch("bib.zotero_import.list_collection")
+    @patch("conf.connect.zotero", return_value=MagicMock())
+    @patch("bib.connect", return_value=MagicMock())
+    def test_only_filters_by_title_substring(
+        self, mc_connect, mc_zotero, mc_list, mc_import
+    ):
+        from bib.zotero_import import ZoteroBook
+
+        cpt = ZoteroBook(
+            key="A", title="CPT 2018", url="", year="2017", publisher="", attachments=()
+        )
+        icd = ZoteroBook(
+            key="B",
+            title="ICD-10-CM 2018",
+            url="",
+            year="2017",
+            publisher="",
+            attachments=(),
+        )
+        mc_list.return_value = [cpt, icd]
+        mc_import.return_value = {
+            "created": 0,
+            "updated": 0,
+            "unchanged": 0,
+            "attached": 0,
+            "missing": 0,
+            "items": [],
+        }
+
+        runner.invoke(
+            app,
+            [
+                "import-zotero",
+                "--collection",
+                "X",
+                "--only",
+                "cpt",
+                "--dry-run",
+            ],
+        )
+
+        passed_books = mc_import.call_args[0][1]
+        assert [b.key for b in passed_books] == ["A"]
+
+    @patch("bib.zotero_import.import_books")
+    @patch("bib.zotero_import.list_collection", return_value=[])
+    @patch("conf.connect.zotero", return_value=MagicMock())
+    @patch("bib.connect", return_value=MagicMock())
+    def test_tags_passed_through(self, mc_connect, mc_zotero, mc_list, mc_import):
+        mc_import.return_value = {
+            "created": 0,
+            "updated": 0,
+            "unchanged": 0,
+            "attached": 0,
+            "missing": 0,
+            "items": [],
+        }
+
+        runner.invoke(
+            app,
+            [
+                "import-zotero",
+                "--collection",
+                "X",
+                "--tag",
+                "source:ama",
+                "--tag",
+                "module:coding",
+            ],
+        )
+
+        _, kwargs = mc_import.call_args
+        assert kwargs["tags"] == ["source:ama", "module:coding"]
+
+    @patch("bib.zotero_import.import_books")
+    @patch("bib.zotero_import.list_collection")
+    @patch("conf.connect.zotero", return_value=MagicMock())
+    @patch("bib.connect", return_value=MagicMock())
+    def test_missing_file_printed_to_stderr(
+        self, mc_connect, mc_zotero, mc_list, mc_import
+    ):
+        from bib.zotero_import import ZoteroBook
+
+        book = ZoteroBook(
+            key="G1", title="Ghost", url="", year="2020", publisher="", attachments=()
+        )
+        mc_list.return_value = [book]
+        mc_import.return_value = {
+            "created": 1,
+            "updated": 0,
+            "unchanged": 0,
+            "attached": 0,
+            "missing": 1,
+            "items": [
+                {
+                    "key": "GKEY",
+                    "zotero_key": "G1",
+                    "title": "Ghost",
+                    "status": "created",
+                    "attached": 0,
+                    "missing": 1,
+                    "missing_paths": ["/data/storage/XX/gone.pdf"],
+                }
+            ],
+        }
+
+        result = runner.invoke(app, ["import-zotero", "--collection", "X"])
+
+        assert "! missing on disk: /data/storage/XX/gone.pdf" in result.output
+        assert "missing=1" in result.output
+
+
 class TestAllHelp:
     """Exercise every registered command's --help (exercises Typer paths)."""
 
@@ -79,6 +249,9 @@ class TestAllHelp:
     def test_sync_zotero(self):
         assert runner.invoke(app, ["sync-zotero", "--help"]).exit_code == 0
 
+    def test_import_zotero(self):
+        assert runner.invoke(app, ["import-zotero", "--help"]).exit_code == 0
+
     def test_backfill(self):
         assert runner.invoke(app, ["backfill-comments", "--help"]).exit_code == 0
 

tests/llm/test_source.py

@@ -135,7 +135,7 @@ class TestCorpusDocs:
 
     def test_llm_skip_tag_excludes_item_from_corpus(self, store):
         key = store.create(
-            Item(item_type="source", title="CPT edition", abstract="Descriptor text.")
+            Item(item_type="source", title="Scratch export", abstract="Draft notes.")
         )
         store.add_tag(key, "llm:skip")
         assert key not in {d.key for d in iter_corpus_docs(store)}