From ab621fc5f566b27f995067d82dfc7949bb3a0d80 Mon Sep 17 00:00:00 2001
From: Tarik Moussa <tarikmoussa@MacBookAir.fritz.box>
Date: Sat, 27 Jun 2026 09:09:39 +0200
Subject: [PATCH] test(review): cover findings #1 (non-bare caller id) and #4
 (backfill connection scope)

#1: assert a URL-form / mixed-case caller id is normalized to the bare canonical form in the papers upsert (the fix's whole point; existing DQ-5 test only used a bare caller). #4: assert ra_grobid_backfill.main() opens two separate connections (read released before the GROBID loop, fresh one for the write) rather than holding one across the loop. Both pass; ruff clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---
 tests/ingest/test_ingest.py              | 30 ++++++++++++++++++
 tests/scripts/test_ra_grobid_backfill.py | 40 ++++++++++++++++++++++--
 2 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/tests/ingest/test_ingest.py b/tests/ingest/test_ingest.py
index 06e4b36..4779f7e 100644
--- a/tests/ingest/test_ingest.py
+++ b/tests/ingest/test_ingest.py
@@ -829,6 +829,36 @@ def test_ingest_doi_paper_upserts_bare_canonical_id() -> None:
     )
 
 
+def test_ingest_normalizes_non_bare_caller_id() -> None:
+    """Review #1: a non-bare CALLER id (URL-form / mixed-case DOI) is normalized to the
+    bare canonical form before being pinned to papers.id (the C-7 pin runs it through
+    _norm_cited_id), so a sloppy caller cannot store a URL-form papers.id that would
+    defeat ON CONFLICT (id) idempotency or the ``paper.id.startswith('10.')`` gates."""
+    paper = _make_paper(paper_id="10.1007/s00454-019-00132-8", openalex_id="W1")
+    mock_conn = MagicMock()
+    mock_conn.execute = MagicMock()
+    mock_conn.executemany = MagicMock()
+    mock_conn.commit = MagicMock()
+
+    with (
+        patch("codex.ingest.openalex.fetch_paper", return_value=paper),
+        patch("codex.ingest.openalex.fetch_citations", return_value=[]),
+        patch("codex.ingest.semanticscholar.fetch_references", return_value=[]),
+        patch("codex.ingest.crossref.fetch_references", return_value=[]),
+        patch("codex.ingest.get_embedder", return_value=_fake_embedder()),
+        patch("codex.ingest.get_conn", side_effect=_make_conn_cm(mock_conn)),
+    ):
+        # Caller passes the URL form with an upper-case suffix char (worst case).
+        result = ingest_paper("https://doi.org/10.1007/S00454-019-00132-8")
+
+    bare = "10.1007/s00454-019-00132-8"
+    assert result.paper_id == bare
+    upsert_params: dict[str, Any] = mock_conn.execute.call_args_list[0][0][1]
+    assert upsert_params["id"] == bare, (
+        f"non-bare caller must be normalized to '{bare}', got '{upsert_params['id']}'"
+    )
+
+
 def test_ingest_disambiguates_bibkey_on_unique_violation() -> None:
     """A papers.bibkey UNIQUE collision is retried with an a/b/c suffix (audit C-15)."""
     paper = _make_paper()  # bibkey auto-generates to "AliceBob2023"
diff --git a/tests/scripts/test_ra_grobid_backfill.py b/tests/scripts/test_ra_grobid_backfill.py
index 8e97b2f..2ef8a6a 100644
--- a/tests/scripts/test_ra_grobid_backfill.py
+++ b/tests/scripts/test_ra_grobid_backfill.py
@@ -6,10 +6,12 @@ Only the pure, offline logic is covered (id normalization + Citation assembly);
 
 from __future__ import annotations
 
-from unittest.mock import patch
+from contextlib import contextmanager
+from typing import Any
+from unittest.mock import MagicMock, patch
 
 from codex.models import Citation
-from scripts.ra_grobid_backfill import _normalize_cited_id, build_grobid_citations
+from scripts.ra_grobid_backfill import _normalize_cited_id, build_grobid_citations, main
 
 # ---------------------------------------------------------------------------
 # _normalize_cited_id
@@ -74,3 +76,37 @@ def test_build_citations_normalizes_dedups_and_drops_self() -> None:
 def test_build_citations_empty_when_no_refs() -> None:
     with patch("scripts.ra_grobid_backfill.extract_references", return_value=[]):
         assert build_grobid_citations("10.1/x", "x.pdf") == []
+
+
+# ---------------------------------------------------------------------------
+# main — connection scope (review finding #4)
+# ---------------------------------------------------------------------------
+
+
+def test_main_uses_separate_connections_for_read_and_write(tmp_path: Any) -> None:
+    """Review #4: the read connection is released BEFORE the slow GROBID loop, and a
+    fresh connection is opened for the write — so no idle-in-transaction connection
+    spans the per-paper network loop (which would abort on an SSH-tunnel drop)."""
+    (tmp_path / "paper.pdf").write_bytes(b"%PDF-1.4")
+    conns: list[MagicMock] = []
+
+    @contextmanager
+    def fake_get_conn() -> Any:
+        c = MagicMock()
+        c.execute.return_value.fetchall.return_value = [
+            {"id": "10.1/x", "source_path": "/d/paper.txt", "out_edges": 0}
+        ]
+        c.execute.return_value.fetchone.return_value = {"papers": 1, "citing": 1, "edges": 1}
+        conns.append(c)
+        yield c
+
+    with (
+        patch("scripts.ra_grobid_backfill.get_conn", side_effect=fake_get_conn),
+        patch("scripts.ra_grobid_backfill.build_grobid_citations", return_value=[]),
+    ):
+        rc = main(["--pdf-dir", str(tmp_path), "--write"])
+
+    assert rc == 0
+    # Two DISTINCT get_conn() context managers (read, then write) — not one held
+    # open across the GROBID loop.
+    assert len(conns) == 2