fix(openalex): normalize DOI to bare canonical id so re-ingest is idempotent (DQ-5)

openalex._map_paper set Paper.id from the raw `doi` field, which OpenAlex returns as a full URL (https://doi.org/10.x). The canonical papers.id (M-1 migration) is the bare, lower-cased DOI, so re-ingesting a DOI paper missed ON CONFLICT (id) and tripped the separate papers_openalex_id_key constraint (UniqueViolation). Fix: _normalize_doi() strips https://doi.org//http://doi.org//doi: and lower-cases; applied in the DOI branch of _map_paper only (W-id fallback untouched). Also un-breaks _s2_id_for/Crossref recovery, which the URL form silently defeated. Corrected the _SAMPLE_WORK fixture to the URL form (the bare fixture hid the bug) + added _normalize_doi and re-ingest idempotency regression tests. Live verification: re-ingest of 10.1007/s00454-019-00132-8 (previously crashed) now UPDATEs in place — single row, added_at unchanged, no stray row, citations 45->45. Unblocks roadmap R-A/R-C. Full suite 348 green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-06-17 00:31:01 +02:00
parent fa43f69dfd
commit ea73b1475c
4 changed files with 181 additions and 15 deletions
--- a/codex/sources/openalex.py
+++ b/codex/sources/openalex.py
@@ -85,13 +85,34 @@ def _reconstruct_abstract(inverted_index: dict[str, list[int]] | None) -> str |
    return " ".join(w for _, w in word_positions)


+def _normalize_doi(doi: str) -> str:
+    """Normalize an OpenAlex DOI to the canonical bare, lower-cased form.
+
+    OpenAlex returns the ``doi`` field as a full URL
+    (``https://doi.org/10.1007/s00454-019-00132-8``), but the canonical
+    ``papers.id`` produced by the M-1 migration is the BARE, lower-cased DOI
+    (``10.1007/s00454-019-00132-8``). Stripping the ``https://doi.org/`` /
+    ``doi:`` prefix and lower-casing (DOIs are case-insensitive by spec) makes a
+    re-ingested DOI paper match its stored row, so the ingest
+    ``INSERT … ON CONFLICT (id)`` updates in place instead of attempting a fresh
+    INSERT that trips the separate ``papers_openalex_id_key`` unique constraint
+    (DQ-5). Mirrors the cited-id convention in ``ingest._norm_cited_id``.
+    """
+    s = doi.strip().lower()
+    for prefix in ("https://doi.org/", "http://doi.org/", "doi:"):
+        if s.startswith(prefix):
+            return s[len(prefix) :]
+    return s
+
+
 def _map_paper(data: dict[str, Any]) -> Paper:
    authors: list[str] = [
        a.get("author", {}).get("display_name") or "" for a in data.get("authorships", [])
    ]
    abstract = _reconstruct_abstract(data.get("abstract_inverted_index"))
+    doi = data.get("doi")
    return Paper(
-        id=data.get("doi") or data.get("id") or "",
+        id=_normalize_doi(doi) if doi else (data.get("id") or ""),
        title=data.get("title") or "",
        openalex_id=data.get("id"),
        authors=authors,
--- a/docs/audit/DATA-QUALITY-2026-06-15.md
+++ b/docs/audit/DATA-QUALITY-2026-06-15.md
@@ -6,8 +6,9 @@ strong after fixing a P0 `codex search paper` crash. **DQ-2 RESOLVED 2026-06-16*
 (1911.00966 fully recovered from arXiv; s00454 abstract from S2; book chapter is a
 documented limit; recovery wired into ingest). **DQ-3 RESOLVED 2026-06-16**
 (content fidelity clean — full coverage, only OCR junk dropped). **All four
-assessment items (DQ-1..DQ-4) DONE. New: DQ-5** (`ingest_paper` not idempotent
-for DOI papers — blocks R-A/R-C, tracked separately).
+assessment items (DQ-1..DQ-4) DONE. DQ-5 RESOLVED 2026-06-17** (DOI id normalized
+to bare canonical form in `openalex._map_paper`; live re-ingest of
+`10.1007/s00454-019-00132-8` confirmed idempotent → **unblocks R-A/R-C**).
 **Roadmap R-A..R-E** added 2026-06-16 (free-source acquisition levers — see Roadmap section).
 **Author:** Audit-Loop (Opus). **Intended reader:** a *cold* session with no memory
 of the audit conversation — everything needed to continue is in this file.
@@ -239,7 +240,43 @@ is fine (monotonic); only the absolute score label is misleading.
 - **Acceptance:** a short relevance table (query → top-5 → on/off-topic) good
  enough to trust the KB for lookups, or a list of failure modes to fix.

-### DQ-5 — `ingest_paper` is NOT idempotent for DOI papers · **HIGH (blocks R-A/R-C)** · found 2026-06-16
+### DQ-5 — `ingest_paper` is NOT idempotent for DOI papers · **RESOLVED 2026-06-17** (found 2026-06-16)
+
+**Resolution (what was done):**
+- **Root-cause fix, minimal + contained:** added `openalex._normalize_doi()` and
+  applied it in `_map_paper` so the DOI branch yields the canonical **bare,
+  lower-cased** form (strips `https://doi.org/` / `http://doi.org/` / `doi:`;
+  lower-cases — DOIs are case-insensitive, matching `ingest._norm_cited_id`). The
+  no-DOI fallback (OpenAlex W-id URL) is left untouched. `_map_paper`→`fetch_paper`
+  →`ingest_paper` is the only consumer chain, so blast radius is one function.
+- **Checked nothing depended on the URL form.** It was the opposite: the full-URL
+  id silently *defeated* `_s2_id_for(paper.id)` and `_recover_abstract`'s
+  `paper.id.startswith("10.")` (a `https://…` string is neither `10.`- nor
+  `doi:`-prefixed), so S2/Crossref recovery never fired for DOI papers. The fix
+  repairs those paths too.
+- **Tests (offline, mocked DB):** closed audit **T-3** — the OpenAlex fixture now
+  emits the real URL-form DOI and asserts `paper.id` is the bare form; added
+  `_normalize_doi` unit tests (https/http/`doi:`/upper-case/already-bare) and a
+  W-id-fallback test; added ingest regression `test_ingest_doi_paper_upserts_bare
+  _canonical_id` (runs the real `_map_paper` through ingest, asserts the papers
+  upsert carries the **bare** id into `%(id)s` and uses `ON CONFLICT (id)`).
+  Verified it has teeth (fails on the pre-fix code). Full suite **348 passed**;
+  ruff + mypy --strict clean.
+- **Live idempotency confirmed (tunnel up):** re-ingested
+  `10.1007/s00454-019-00132-8` against the Jetson DB → no `UniqueViolation`;
+  `IngestResult(paper_id='10.1007/s00454-019-00132-8', citations_upserted=45)`.
+  After-state: **UPDATE in place** — `id` and `openalex_id` unchanged, **`added_at`
+  unchanged** (proves no fresh INSERT), exactly **1** row for the DOI (no URL-form
+  duplicate), citations 45→45, total papers 29→29.
+- **Unblocks R-A (GROBID re-ingest) and R-C (.tex re-ingest)** — both re-ingest
+  existing papers. **Files touched:** `codex/sources/openalex.py`,
+  `tests/sources/test_openalex.py`, `tests/ingest/test_ingest.py`.
+- **Note (out of scope, pre-existing):** arXiv papers stored as bare ids
+  (`2305.10988`) are still not round-trip idempotent if OpenAlex returns an
+  arXiv-DOI/W-id instead of the bare arXiv id — a separate canonicalization
+  concern, not this UniqueViolation. The DOI path (this finding) is fixed.
+
+**Original finding (for context):**
 - **Symptom:** re-ingesting an existing DOI paper crashes with
  `UniqueViolation: papers_openalex_id_key`. Surfaced trying to re-ingest
  `10.1007/s00454-019-00132-8` during the DQ-2 backfill.
@@ -276,8 +313,8 @@ Paywall/uni-login was explicitly considered and **deferred** (see R-E). Each ite
 is self-contained so a cold session can pick it up.

 ### R-A — Run GROBID reference extraction on arXiv PDFs · **HIGH lever, LOW-MED effort**
- **⚠ Blocked by DQ-5:** re-ingesting existing DOI papers currently crashes
-  (`papers_openalex_id_key`). Fix DQ-5 first, or the GROBID re-ingest aborts.
+- **✓ Unblocked (DQ-5 RESOLVED 2026-06-17):** DOI re-ingest is now idempotent, so
+  the GROBID re-ingest no longer aborts on `papers_openalex_id_key`.
 - **What:** the whole corpus was ingested from `.txt` (`PAPERS_DIR=…/papers/txt`),
  so the GROBID PDF path never ran. Re-ingest with a PDF `source_path` per paper
  so `codex.parsing.grobid.extract_references` parses each bibliography.
@@ -350,13 +387,13 @@ is self-contained so a cold session can pick it up.
   from S2; book chapter documented; recovery wired into ingest).
 4. ~~**DQ-3**~~ — **DONE 2026-06-16** (content fidelity clean; full coverage, only
   18 OCR-junk chunks dropped, no math lost). **Assessment axis DQ-1..DQ-4 complete.**
-5. **DQ-5** (NEXT — `ingest_paper` not idempotent for DOI papers; **blocks R-A/R-C**;
-   careful — M-1 canonical-id territory).
-6. **Roadmap levers** (see section above): **R-A** (GROBID on arXiv PDFs — closes
-   the last 2 zero-citation theses; needs DQ-5 first), **R-B** (Crossref refs —
-   abstract half already shipped in DQ-2), **R-C** (`.tex` ingest — serves DQ-3;
-   needs DQ-5 first), **R-D** (F-15 coverage warning, quick). **R-E** (paywall)
-   deferred.
+5. ~~**DQ-5**~~ — **DONE 2026-06-17** (DOI id normalized to bare form in
+   `openalex._map_paper`; live re-ingest of `10.1007/s00454-019-00132-8` idempotent
+   — UPDATE in place, `added_at` unchanged; suite 348 green). **Unblocks R-A/R-C.**
+6. **Roadmap levers** (see section above — now all unblocked): **R-A** (GROBID on
+   arXiv PDFs — closes the last 2 zero-citation theses), **R-B** (Crossref refs —
+   abstract half already shipped in DQ-2), **R-C** (`.tex` ingest — serves DQ-3),
+   **R-D** (F-15 coverage warning, quick). **R-E** (paywall) deferred.

 DQ-1..DQ-4 are *data*/assessment work; DQ-5 + roadmap R-A..R-D add **new code**
 (canonical-id fix, re-ingest scripts, Crossref references, a `.tex` path, a graph
--- a/tests/ingest/test_ingest.py
+++ b/tests/ingest/test_ingest.py
@@ -10,6 +10,7 @@ from contextlib import contextmanager
 from typing import Any
 from unittest.mock import MagicMock, patch

+import httpx
 import numpy as np
 import pytest

@@ -592,6 +593,63 @@ def test_ingest_sets_bibkey_when_none(tmp_path: Any) -> None:
    assert upsert_params["bibkey"] == "AliceBob2023"


+def test_ingest_doi_paper_upserts_bare_canonical_id() -> None:
+    """DQ-5 regression: re-ingesting an existing DOI paper must be idempotent.
+
+    OpenAlex returns the ``doi`` as a full URL (and possibly mixed-case), but the
+    stored canonical ``papers.id`` is the BARE, lower-cased DOI (M-1 migration).
+    The papers upsert must therefore carry the BARE id into ``%(id)s`` so
+    ``ON CONFLICT (id)`` matches the existing row and UPDATEs it — rather than
+    attempting a fresh INSERT that trips the separate ``papers_openalex_id_key``
+    unique constraint (the UniqueViolation crash).
+
+    The real ``_map_paper``/``fetch_paper`` run here (only the HTTP ``_get``, the
+    embedder and the DB are mocked), so this exercises the actual normalization
+    through the ingest path. The DB is a MagicMock, so it cannot raise the real
+    constraint error; the live re-ingest of 10.1007/s00454-019-00132-8 is the
+    end-to-end UniqueViolation proof (see DATA-QUALITY-2026-06-15.md, DQ-5).
+    """
+    bare = "10.1007/s00454-019-00132-8"
+    work = {
+        "id": "https://openalex.org/W2899262408",
+        # URL form + an upper-case suffix char — the worst case for ON CONFLICT.
+        "doi": "https://doi.org/10.1007/S00454-019-00132-8",
+        "title": "A Test Paper",
+        "publication_year": 2019,
+        "authorships": [{"author": {"display_name": "Alice Smith"}}],
+        "abstract_inverted_index": {"Hello": [0], "world": [1]},
+        # Non-empty so fetch_citations yields edges and the S2 fallback is skipped.
+        "referenced_works": ["https://openalex.org/W1"],
+    }
+
+    mock_conn = MagicMock()
+    mock_conn.execute = MagicMock()
+    mock_conn.executemany = MagicMock()
+    mock_conn.commit = MagicMock()
+
+    with (
+        patch(
+            "codex.ingest.openalex._get",
+            return_value=httpx.Response(200, json=work),
+        ),
+        patch("codex.ingest.get_embedder", return_value=_fake_embedder()),
+        patch("codex.ingest.get_conn", side_effect=_make_conn_cm(mock_conn)),
+    ):
+        result = ingest_paper(bare)
+
+    # The IngestResult and the upsert must use the bare canonical id, not the URL.
+    assert result.paper_id == bare
+
+    upsert_sql: str = mock_conn.execute.call_args_list[0][0][0]
+    upsert_params: dict[str, Any] = mock_conn.execute.call_args_list[0][0][1]
+    assert "INSERT INTO papers" in upsert_sql
+    assert "ON CONFLICT (id)" in upsert_sql
+    assert upsert_params["id"] == bare, (
+        f"upsert id must be the bare canonical DOI '{bare}', "
+        f"not the OpenAlex URL form '{upsert_params['id']}'"
+    )
+
+
 # ---------------------------------------------------------------------------


--- a/tests/sources/test_openalex.py
+++ b/tests/sources/test_openalex.py
@@ -7,7 +7,7 @@ import pytest

 from codex.models import Citation, Paper
 from codex.sources import openalex
-from codex.sources.openalex import _resolve_id
+from codex.sources.openalex import _normalize_doi, _resolve_id

 # ---------------------------------------------------------------------------
 # Helpers
@@ -15,7 +15,9 @@ from codex.sources.openalex import _resolve_id

 _SAMPLE_WORK = {
    "id": "https://openalex.org/W2741809807",
-    "doi": "10.1145/3592430",
+    # Real OpenAlex returns the DOI as a full URL (T-3: the old bare fixture hid
+    # the URL-form behaviour that DQ-5 is about).
+    "doi": "https://doi.org/10.1145/3592430",
    "title": "Conformal Prediction: A Review",
    "publication_year": 2023,
    "authorships": [
@@ -67,6 +69,51 @@ def test_resolve_prefixed_unchanged() -> None:
    assert _resolve_id("doi:10.1145/x") == "doi:10.1145/x"


+# ---------------------------------------------------------------------------
+# _normalize_doi — canonical bare, lower-cased form (DQ-5)
+# ---------------------------------------------------------------------------
+
+
+def test_normalize_doi_strips_https_url() -> None:
+    # The exact form OpenAlex returns in the `doi` field.
+    assert _normalize_doi("https://doi.org/10.1007/s00454-019-00132-8") == (
+        "10.1007/s00454-019-00132-8"
+    )
+
+
+def test_normalize_doi_strips_http_and_doi_prefix() -> None:
+    assert _normalize_doi("http://doi.org/10.1145/3592430") == "10.1145/3592430"
+    assert _normalize_doi("doi:10.1145/3592430") == "10.1145/3592430"
+
+
+def test_normalize_doi_lowercases() -> None:
+    # DOIs are case-insensitive; the canonical id (and _norm_cited_id) is lower-case.
+    assert _normalize_doi("https://doi.org/10.1007/S00454-019-00132-8") == (
+        "10.1007/s00454-019-00132-8"
+    )
+
+
+def test_normalize_doi_already_bare_is_idempotent() -> None:
+    assert _normalize_doi("10.1007/s00454-019-00132-8") == "10.1007/s00454-019-00132-8"
+
+
+def test_map_paper_falls_back_to_openalex_id_without_doi(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """A work with no DOI keeps the OpenAlex W-id (URL form) as id, unchanged —
+    normalization only rewrites the DOI branch."""
+    work = {k: v for k, v in _SAMPLE_WORK.items() if k != "doi"}
+
+    def mock_get(url: str, params: dict[str, str] | None = None) -> httpx.Response:
+        return httpx.Response(200, json=work)
+
+    monkeypatch.setattr(openalex, "_get", mock_get)
+    paper = openalex.fetch_paper("W2741809807")
+
+    assert paper is not None
+    assert paper.id == "https://openalex.org/W2741809807"
+
+
 # ---------------------------------------------------------------------------
 # fetch_paper — success
 # ---------------------------------------------------------------------------
@@ -85,6 +132,9 @@ def test_fetch_paper_success(monkeypatch: pytest.MonkeyPatch) -> None:

    assert paper is not None
    assert isinstance(paper, Paper)
+    # DQ-5 / C-7: paper.id is the canonical BARE DOI, not the full URL OpenAlex
+    # returns. This is what makes the ingest ON CONFLICT (id) upsert idempotent.
+    assert paper.id == "10.1145/3592430"
    assert paper.title == "Conformal Prediction: A Review"
    assert paper.year == 2023
    assert paper.authors == ["Alice Smith", "Bob Jones"]