From b2f01ce0108a4cec5f8bddb17a369b62d2433611 Mon Sep 17 00:00:00 2001 From: Tarik Moussa Date: Fri, 26 Jun 2026 22:50:23 +0200 Subject: [PATCH] fix(review): address PR #16 code-review findings #1 ingest: normalize the pinned caller id (_norm_cited_id) so a non-bare DOI caller cannot store a URL-form papers.id that defeats DQ-5 idempotency / the startswith(10.) recovery gates. #2 quality.section_label: collapse to a controlled bucket only for an EXACT canonical heading; descriptive titles ('Abstract Nonsense...') keep their real title instead of being mislabelled. #4 ra_grobid_backfill: release the read connection before the slow GROBID network loop, fresh connection for the write (no idle-in-transaction across the loop over the flaky tunnel). #5/#10 tex: flatten_inputs strips unresolved input/include at the depth cap (no literal leak on cycles); _norm_texkey strips only a single leading ./ . #6/#7 arxiv.fetch_source: keep non-.tex members resolvable for input; pick primary on an UN-commented documentclass line. #13 is_arxiv_id: also exclude http:// and arXiv-DOI forms. Tests added/updated for each. Left as deliberate decisions: #3 (pre-section text drop is pre-existing in extract_sections; abstract stored separately), #8 (script normalizer is intentionally self-contained, already documented), #9/#11/#12 (no current trigger / tightening the gzip heuristic would reject valid old LaTeX like documentstyle / title truncation is by design on a write-only column). ruff + mypy clean. Co-Authored-By: Claude Opus 4.8 --- codex/ingest.py | 14 ++-- codex/parsing/tex.py | 8 ++- codex/quality.py | 33 ++++++++-- codex/sources/arxiv.py | 33 ++++++++-- scripts/ra_grobid_backfill.py | 95 ++++++++++++++------------- scripts/rc_tex_reingest.py | 8 ++- tests/ingest/test_ingest.py | 12 ++-- tests/parsing/test_tex.py | 12 ++++ tests/quality/test_quality.py | 17 +++-- tests/scripts/test_rc_tex_reingest.py | 2 + tests/sources/test_arxiv.py | 31 +++++++++ 11 files changed, 190 insertions(+), 75 deletions(-) diff --git a/codex/ingest.py b/codex/ingest.py index 6518d74..9381c69 100644 --- a/codex/ingest.py +++ b/codex/ingest.py @@ -207,12 +207,14 @@ def ingest_paper( raise ValueError(f"Paper not found: {paper_id}") # Canonical identity is the caller-supplied id — what the CLI / ingest scripts - # use and what papers.id is contracted to hold (arXiv id or DOI). OpenAlex's - # fetch_paper fills paper.id with the URL-form doi/id (audit C-7); since DQ-5 - # `openalex._canonical_id` already normalizes that to the bare form, this pin - # is belt-and-suspenders and keeps re-ingest idempotent regardless. The - # OpenAlex work id is retained as openalex_id and in paper_identifiers. - paper.id = paper_id.strip() + # use and what papers.id is contracted to hold (arXiv id or DOI). Pin it to the + # caller's id (audit C-7), but normalize it the same way cited-ids are (strip a + # https://doi.org/ or doi: prefix, lower-case DOIs) so a non-bare caller cannot + # store a URL-form papers.id that would defeat DQ-5's ON CONFLICT (id) idempotency + # or the `paper.id.startswith("10.")` recovery gates below. arXiv/W ids pass + # through unchanged. The OpenAlex work id is retained as openalex_id / in + # paper_identifiers. + paper.id = _norm_cited_id(paper_id.strip()) # DQ-2: OpenAlex often has the paper but no abstract (it cannot redistribute # some publishers' abstracts). Supplement from S2, then Crossref (uses the diff --git a/codex/parsing/tex.py b/codex/parsing/tex.py index ed77ad7..afc5b02 100644 --- a/codex/parsing/tex.py +++ b/codex/parsing/tex.py @@ -170,7 +170,9 @@ def _norm_texkey(name: str) -> str: ``.tex`` so ``\\input{sections/intro}`` matches the archive member ``sections/intro.tex``. """ - n = name.strip().lstrip("./") + n = name.strip() + if n.startswith("./"): # a single leading "./" only — don't eat "../" or ".hidden" + n = n[2:] return n[:-4] if n.endswith(".tex") else n @@ -188,7 +190,9 @@ def flatten_inputs(primary: str, files: dict[str, str]) -> str: def _expand(text: str, depth: int) -> str: if depth > 10: - return text + # Cap reached (likely an \input cycle): strip any still-unresolved + # directives rather than leak a literal "\input{…}" into the parsed text. + return _INPUT_RE.sub("", text) def _repl(match: re.Match[str]) -> str: content = norm.get(_norm_texkey(match.group(1))) diff --git a/codex/quality.py b/codex/quality.py index 7dc21f8..2bd23fc 100644 --- a/codex/quality.py +++ b/codex/quality.py @@ -127,14 +127,34 @@ def _clean_title(title: str) -> str: return t[:60].rsplit(" ", 1)[0] if len(t) > 60 else t +# A heading collapses to a controlled bucket only when it IS one of these canonical +# section words — NOT when it merely starts with one. Prefix-matching via +# classify_section mislabels real sections ("Abstract Nonsense and Categories" → +# abstract, "References Architecture" → bibliography); an exact map avoids that while +# keeping cross-source consistency for the bare headings. +_SECTION_TITLE_BUCKETS = { + "abstract": "abstract", + "zusammenfassung": "abstract", + "introduction": "intro", + "einleitung": "intro", + "proof": "proof", + "beweis": "proof", + "references": "bibliography", + "bibliography": "bibliography", + "bibliographie": "bibliography", + "literatur": "bibliography", +} + + def section_label(title: str | None, content: str) -> str: """Label a chunk's section, preferring the real ``\\section`` heading (R-F). - For section-aware ``.tex`` ingest *title* is the real heading: if it maps to a - controlled bucket via :func:`classify_section` (intro / theorem / proof / - abstract / bibliography) that bucket is kept (cross-source consistency); - otherwise the cleaned title itself is stored (e.g. ``"preliminaries"``, - ``"rigidity"``) — far more signal than collapsing everything to ``body``. + For section-aware ``.tex`` ingest *title* is the real heading: a bare canonical + heading ("Introduction", "References", "Proof") maps to its controlled bucket + (cross-source consistency); any other heading is stored as its cleaned real + title (e.g. ``"preliminaries"``, ``"introduction to operator algebras"``) — far + more signal than collapsing to ``body`` AND without the prefix-match mislabels + that ``classify_section`` would produce on descriptive titles. When *title* is None / blank (the ``.pdf`` / ``.txt`` / ``run_quality_pass`` paths) this falls back to content-based :func:`classify_section` — unchanged @@ -146,8 +166,7 @@ def section_label(title: str | None, content: str) -> str: cleaned = _clean_title(title) if not cleaned: return classify_section(content) - bucket = classify_section(cleaned) - return bucket if bucket != "body" else cleaned + return _SECTION_TITLE_BUCKETS.get(cleaned, cleaned) # --------------------------------------------------------------------------- diff --git a/codex/sources/arxiv.py b/codex/sources/arxiv.py index 80b6932..2452172 100644 --- a/codex/sources/arxiv.py +++ b/codex/sources/arxiv.py @@ -97,6 +97,16 @@ def fetch_metadata(arxiv_id: str) -> Paper | None: ) +def _has_documentclass(content: str) -> bool: + """True if *content* has an UN-commented ``\\documentclass`` line. + + A plain ``"\\documentclass" in content`` substring test also matches a + commented-out ``%\\documentclass`` (common in arXiv preambles); requiring the + command to start its line avoids selecting the wrong primary file. + """ + return any(line.lstrip().startswith("\\documentclass") for line in content.splitlines()) + + def fetch_source(arxiv_id: str) -> str | None: """Download and extract the primary LaTeX source for an arXiv paper. @@ -128,29 +138,38 @@ def fetch_source(arxiv_id: str) -> str | None: from codex.parsing.tex import flatten_inputs raw = response.content + # Image/font/binary members are never \input'd as text; skip them so the source + # map stays text-only (decode-with-replace would otherwise store garbled bytes). + skip_ext = (".png", ".jpg", ".jpeg", ".gif", ".bmp", ".pdf", ".eps", ".ps", ".ttf", ".otf") try: with tarfile.open(fileobj=io.BytesIO(raw), mode="r:gz") as tf: files: dict[str, str] = {} primary_name: str | None = None for member in tf.getmembers(): - if not member.name.endswith(".tex"): + if not member.isfile() or member.name.lower().endswith(skip_ext): continue f = tf.extractfile(member) if f is None: continue - content = f.read().decode("utf-8", errors="replace") - files[member.name] = content - # Prefer the \documentclass file as the primary document. - if primary_name is None and "\\documentclass" in content: + # Keep ALL text members resolvable so \input of a non-.tex include + # (e.g. a .def or extension-less body file) is not silently dropped (R-C). + files[member.name] = f.read().decode("utf-8", errors="replace") + # Primary doc = a .tex with an UN-commented \documentclass. + if ( + primary_name is None + and member.name.endswith(".tex") + and _has_documentclass(files[member.name]) + ): primary_name = member.name - if not files: + tex_files = {k: v for k, v in files.items() if k.endswith(".tex")} + if not tex_files: logger.debug("No .tex files found in arXiv source for %s", arxiv_id) return None # No \documentclass anywhere → fall back to the largest .tex. if primary_name is None: - primary_name = max(files, key=lambda k: len(files[k])) + primary_name = max(tex_files, key=lambda k: len(tex_files[k])) # Inline \input/\include so multi-file projects yield the full body, # not just the primary file's skeleton of include directives (R-C). diff --git a/scripts/ra_grobid_backfill.py b/scripts/ra_grobid_backfill.py index 798230e..5f8ed04 100644 --- a/scripts/ra_grobid_backfill.py +++ b/scripts/ra_grobid_backfill.py @@ -142,6 +142,10 @@ def main(argv: list[str] | None = None) -> int: pdf_dir = Path(args.pdf_dir) pdfs = {p.stem: p for p in pdf_dir.glob("*.pdf")} + # Read the target list in a short-lived connection, then release it: the GROBID + # extraction below is a slow per-paper network loop and must NOT hold an + # idle-in-transaction connection open across it (a mid-loop SSH-tunnel drop would + # otherwise abort the whole batch — see docs/infra/jetson-ssh-stability.md). with get_conn() as conn: rows = conn.execute( """ @@ -152,46 +156,49 @@ def main(argv: list[str] | None = None) -> int: """ ).fetchall() - targets: list[tuple[str, Path]] = [] - for r in rows: - if args.only and r["id"] != args.only: - continue - if args.zero_edge_only and r["out_edges"] != 0: - continue - stem = Path(r["source_path"]).stem if r["source_path"] else None - pdf = pdfs.get(stem) if stem else None - if pdf is None: - logger.warning("no PDF for %s (stem=%s) — skipping", r["id"], stem) - continue - targets.append((r["id"], pdf)) - if args.limit is not None: - targets = targets[: args.limit] + targets: list[tuple[str, Path]] = [] + for r in rows: + if args.only and r["id"] != args.only: + continue + if args.zero_edge_only and r["out_edges"] != 0: + continue + stem = Path(r["source_path"]).stem if r["source_path"] else None + pdf = pdfs.get(stem) if stem else None + if pdf is None: + logger.warning("no PDF for %s (stem=%s) — skipping", r["id"], stem) + continue + targets.append((r["id"], pdf)) + if args.limit is not None: + targets = targets[: args.limit] - logger.info( - "GROBID=%s | papers to process=%d | mode=%s", - grobid_url, - len(targets), - "WRITE" if args.write else "DRY-RUN", - ) + logger.info( + "GROBID=%s | papers to process=%d | mode=%s", + grobid_url, + len(targets), + "WRITE" if args.write else "DRY-RUN", + ) - all_citations: list[Citation] = [] - for paper_id, pdf in targets: - try: - cits = build_grobid_citations( - paper_id, pdf, grobid_url=grobid_url, timeout=args.timeout - ) - except Exception as exc: - logger.warning("GROBID extraction failed for %s (%s): %s", paper_id, pdf.name, exc) - continue - logger.info("%-38s %3d refs (%s)", paper_id, len(cits), pdf.name) - all_citations.extend(cits) + # GROBID extraction — no DB connection held during this slow network loop. + all_citations: list[Citation] = [] + for paper_id, pdf in targets: + try: + cits = build_grobid_citations( + paper_id, pdf, grobid_url=grobid_url, timeout=args.timeout + ) + except Exception as exc: + logger.warning("GROBID extraction failed for %s (%s): %s", paper_id, pdf.name, exc) + continue + logger.info("%-38s %3d refs (%s)", paper_id, len(cits), pdf.name) + all_citations.extend(cits) - logger.info("total candidate citations: %d", len(all_citations)) + logger.info("total candidate citations: %d", len(all_citations)) - if not args.write: - logger.info("DRY-RUN — no rows written. Re-run with --write to apply.") - return 0 + if not args.write: + logger.info("DRY-RUN — no rows written. Re-run with --write to apply.") + return 0 + # Fresh connection for the write — the read connection was already released. + with get_conn() as conn: before = _coverage(conn) with conn.cursor() as cur: cur.executemany( @@ -204,15 +211,15 @@ def main(argv: list[str] | None = None) -> int: ) conn.commit() after = _coverage(conn) - logger.info( - "coverage papers=%d citing %d->%d edges %d->%d (+%d)", - after[0], - before[1], - after[1], - before[2], - after[2], - after[2] - before[2], - ) + logger.info( + "coverage papers=%d citing %d->%d edges %d->%d (+%d)", + after[0], + before[1], + after[1], + before[2], + after[2], + after[2] - before[2], + ) return 0 diff --git a/scripts/rc_tex_reingest.py b/scripts/rc_tex_reingest.py index 53839ac..7ccdf1e 100644 --- a/scripts/rc_tex_reingest.py +++ b/scripts/rc_tex_reingest.py @@ -50,7 +50,13 @@ def is_arxiv_id(paper_id: str) -> bool: with ``10.``; OpenAlex W-ids start with ``W`` or an ``https://openalex.org/`` URL. """ p = (paper_id or "").strip() - return bool(p) and not p.startswith("10.") and not p.startswith(("W", "https://")) + # Exclude DOIs (incl. the arXiv DOI 10.48550/arXiv.*), OpenAlex W-ids, and any + # http(s) URL form; everything else is treated as a bare arXiv id. + return ( + bool(p) + and not p.startswith(("10.", "W")) + and not p.lower().startswith(("http://", "https://")) + ) def _section_dist(conn: psycopg.Connection[psycopg.rows.DictRow], paper_id: str) -> dict[str, int]: diff --git a/tests/ingest/test_ingest.py b/tests/ingest/test_ingest.py index 85b54de..06e4b36 100644 --- a/tests/ingest/test_ingest.py +++ b/tests/ingest/test_ingest.py @@ -238,11 +238,15 @@ def test_ingest_paper_source_tex(tmp_path: Any) -> None: chunk_sql: str = chunk_insert_call[0][0] assert "INSERT INTO chunks" in chunk_sql - # R-F: heading → controlled bucket where it maps ("Introduction"→intro, - # "Proof of…"→proof), else the cleaned real title ("discrete conformal maps") - # instead of collapsing to "body". + # R-F: a bare canonical heading → bucket ("Introduction"→intro); any other + # heading is stored as its cleaned real title ("proof of the main theorem", + # "discrete conformal maps") instead of being collapsed/mislabelled. chunk_rows = chunk_insert_call[0][1] - assert [row[4] for row in chunk_rows] == ["intro", "proof", "discrete conformal maps"] + assert [row[4] for row in chunk_rows] == [ + "intro", + "proof of the main theorem", + "discrete conformal maps", + ] assert result.chunks_upserted == len(section_pairs) diff --git a/tests/parsing/test_tex.py b/tests/parsing/test_tex.py index 13a8044..7e97a5f 100644 --- a/tests/parsing/test_tex.py +++ b/tests/parsing/test_tex.py @@ -146,6 +146,18 @@ class TestFlattenInputs: assert r"\input" not in out assert "A" in out and "B" in out + def test_input_cycle_does_not_leak_directive(self) -> None: + # a -> b -> a: the depth cap must strip the unresolved directive, not leak it. + files = {"a.tex": r"AA \input{b}", "b.tex": r"BB \input{a}"} + out = flatten_inputs(files["a.tex"], files) + assert r"\input" not in out + assert "AA" in out and "BB" in out + + def test_dot_slash_prefix_resolves_without_overstrip(self) -> None: + # "./intro" resolves intro.tex; lstrip must not eat a real leading dot/run. + files = {"main.tex": r"\input{./intro}", "intro.tex": "BODY"} + assert "BODY" in flatten_inputs(files["main.tex"], files) + class TestChunkSections: def test_pairs_carry_section_titles(self) -> None: diff --git a/tests/quality/test_quality.py b/tests/quality/test_quality.py index 4895c30..240d802 100644 --- a/tests/quality/test_quality.py +++ b/tests/quality/test_quality.py @@ -272,16 +272,25 @@ class TestCleanTitle: class TestSectionLabel: - def test_title_maps_to_controlled_bucket(self): - # Headings matching the controlled vocab keep the canonical bucket. + def test_bare_canonical_heading_maps_to_bucket(self): + # Only an EXACT canonical heading collapses to its controlled bucket. assert section_label("Introduction", "body text") == "intro" - assert section_label("Proof of Theorem 3", "body text") == "proof" + assert section_label("Proof", "body text") == "proof" assert section_label("References", "body text") == "bibliography" + assert section_label("Abstract", "body text") == "abstract" def test_descriptive_title_stored_verbatim(self): - # R-F win: a non-vocab heading is stored as its cleaned title, not "body". + # R-F: a heading that merely STARTS with a keyword keeps its real title, + # not the bucket — fixes classify_section prefix-match over-bucketing. assert section_label("Preliminaries", "body text") == "preliminaries" assert section_label("3.1 Discrete Conformal Maps", "x") == "discrete conformal maps" + assert section_label("Proof of Theorem 3", "x") == "proof of theorem 3" + assert section_label("Abstract Nonsense and Categories", "x") == ( + "abstract nonsense and categories" + ) + assert section_label("Introduction to Operator Algebras", "x") == ( + "introduction to operator algebras" + ) def test_none_or_blank_title_falls_back_to_content(self): # .pdf/.txt path: unchanged content-based classification. diff --git a/tests/scripts/test_rc_tex_reingest.py b/tests/scripts/test_rc_tex_reingest.py index 17a8770..3f7239f 100644 --- a/tests/scripts/test_rc_tex_reingest.py +++ b/tests/scripts/test_rc_tex_reingest.py @@ -16,4 +16,6 @@ def test_is_arxiv_id_rejects_dois_and_wids() -> None: assert not is_arxiv_id("10.14279/depositonce-20357") assert not is_arxiv_id("W2971636899") assert not is_arxiv_id("https://openalex.org/W2971636899") + assert not is_arxiv_id("http://openalex.org/W2971636899") + assert not is_arxiv_id("10.48550/arXiv.2305.10988") # arXiv DOI form, not bare id assert not is_arxiv_id("") diff --git a/tests/sources/test_arxiv.py b/tests/sources/test_arxiv.py index db376c1..25e701b 100644 --- a/tests/sources/test_arxiv.py +++ b/tests/sources/test_arxiv.py @@ -110,6 +110,37 @@ def test_fetch_source_bare_gzip_single_file(monkeypatch: pytest.MonkeyPatch) -> assert "Old single-file paper body." in result +def test_fetch_source_resolves_non_tex_include(monkeypatch: pytest.MonkeyPatch) -> None: + """\\input of a non-.tex body member (e.g. a .def file) is inlined, not dropped (R-C).""" + main = b"\\documentclass{article}\\begin{document}\\input{body.def}\\end{document}" + tar_bytes = _make_tar_gz({"main.tex": main, "body.def": b"NON TEX BODY TEXT"}) + + def mock_get(url: str, *, timeout: int = 60, follow_redirects: bool = True) -> httpx.Response: + return httpx.Response(200, content=tar_bytes) + + monkeypatch.setattr(httpx, "get", mock_get) + result = arxiv.fetch_source("2301.07041") + assert result is not None + assert "NON TEX BODY TEXT" in result + + +def test_fetch_source_ignores_commented_documentclass(monkeypatch: pytest.MonkeyPatch) -> None: + """A commented %\\documentclass must not select a file as the primary document.""" + decoy = b"% \\documentclass{article}\n\\section{Stub}\nDECOY ONLY" + real = b"\\documentclass{book}\\begin{document}REAL BODY HERE\\end{document}" + # decoy sorts/inserts first; without the fix it would win on the substring match. + tar_bytes = _make_tar_gz({"aaa_decoy.tex": decoy, "real.tex": real}) + + def mock_get(url: str, *, timeout: int = 60, follow_redirects: bool = True) -> httpx.Response: + return httpx.Response(200, content=tar_bytes) + + monkeypatch.setattr(httpx, "get", mock_get) + result = arxiv.fetch_source("2301.07041") + assert result is not None + assert "REAL BODY HERE" in result + assert "DECOY ONLY" not in result + + # --------------------------------------------------------------------------- # fetch_source — 404 → None # ---------------------------------------------------------------------------