Merge pull request 'fix: audit remediation Wave 2 — identity normalization C-7, C-10 (+ T-3, ADR D-1)' (#13) from fix/audit-wave-2 into main
This commit is contained in:
@@ -98,6 +98,14 @@ def ingest_paper(
|
|||||||
if paper is None:
|
if paper is None:
|
||||||
raise ValueError(f"Paper not found: {paper_id}")
|
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 (e.g.
|
||||||
|
# "https://doi.org/10.48550/arxiv.math/0603097"), which broke bare-id lookups
|
||||||
|
# and cross-citation matching (audit C-7). The OpenAlex work id is retained
|
||||||
|
# separately as openalex_id and in the paper_identifiers alias table.
|
||||||
|
paper.id = paper_id.strip()
|
||||||
|
|
||||||
# Auto-generate bibkey when source has none (D-04).
|
# Auto-generate bibkey when source has none (D-04).
|
||||||
if paper.bibkey is None:
|
if paper.bibkey is None:
|
||||||
paper.bibkey = _make_bibkey(paper)
|
paper.bibkey = _make_bibkey(paper)
|
||||||
@@ -286,7 +294,10 @@ def ingest_paper(
|
|||||||
VALUES (%s, %s, %s, %s, %s)
|
VALUES (%s, %s, %s, %s, %s)
|
||||||
""",
|
""",
|
||||||
[
|
[
|
||||||
(f.paper_id, f.page, f.raw_latex, f.context, f.eq_label)
|
# paper.id, not f.paper_id: the extractor derives its
|
||||||
|
# paper_id from the PDF filename stem, which need not match
|
||||||
|
# papers.id and would violate the FK (audit C-10).
|
||||||
|
(paper.id, f.page, f.raw_latex, f.context, f.eq_label)
|
||||||
for f in formulas
|
for f in formulas
|
||||||
],
|
],
|
||||||
)
|
)
|
||||||
@@ -300,7 +311,8 @@ def ingest_paper(
|
|||||||
VALUES (%s, %s, %s, %s)
|
VALUES (%s, %s, %s, %s)
|
||||||
""",
|
""",
|
||||||
[
|
[
|
||||||
(fig.paper_id, fig.page, fig.caption, fig.image_path)
|
# paper.id, not fig.paper_id (filename stem) — see C-10 above.
|
||||||
|
(paper.id, fig.page, fig.caption, fig.image_path)
|
||||||
for fig in figures
|
for fig in figures
|
||||||
],
|
],
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -1,8 +1,10 @@
|
|||||||
# ADR-F15 — Literature Graph / Citation PageRank
|
# ADR-F15 — Literature Graph / Citation PageRank
|
||||||
|
|
||||||
**Status:** IMPL (GO after live-corpus spike 2026-06-15)
|
**Status:** IMPL — GO after live-corpus spike 2026-06-15. See **Audit Update
|
||||||
|
(2026-06-15)** at the end: the ID-format mismatch is now resolved in code
|
||||||
|
(audit C-1, C-7) and the Spike Result numbers below predate that resolution.
|
||||||
**Owners:** F-15 Worker (Sonnet)
|
**Owners:** F-15 Worker (Sonnet)
|
||||||
**Last updated:** 2026-06-15
|
**Last updated:** 2026-06-15 (audit addendum)
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
@@ -138,6 +140,12 @@ If the `citations` table is empty: graph has 0 nodes; `graph report` prints
|
|||||||
|
|
||||||
## Known Limitation: ID-Format Mismatch
|
## Known Limitation: ID-Format Mismatch
|
||||||
|
|
||||||
|
> **RESOLVED (audit 2026-06-15) — see Audit Update below.** This no longer holds:
|
||||||
|
> `build_citation_graph` resolves OpenAlex `cited_id`s to the canonical
|
||||||
|
> `papers.id` via the shared `RESOLVED_CITATIONS_SQL` (C-1) and ingest stores
|
||||||
|
> canonical ids (C-7), so inter-KB citations *are* now edges. Original text kept
|
||||||
|
> for the record.
|
||||||
|
|
||||||
`cited_id` uses OpenAlex IDs; `papers.id` uses DOIs. Cross-citations
|
`cited_id` uses OpenAlex IDs; `papers.id` uses DOIs. Cross-citations
|
||||||
between ingested papers are therefore not represented as graph edges.
|
between ingested papers are therefore not represented as graph edges.
|
||||||
This is a D-05-class issue (same root cause as the `citing_id`/`openalex_id`
|
This is a D-05-class issue (same root cause as the `citing_id`/`openalex_id`
|
||||||
@@ -155,3 +163,27 @@ small while the corpus is < 100 papers.
|
|||||||
- R-44: `codex graph report [--top-n N] [--json]` ✓
|
- R-44: `codex graph report [--top-n N] [--json]` ✓
|
||||||
- R-45: graceful degradation < 5 nodes: uniform scores, warning ✓
|
- R-45: graceful degradation < 5 nodes: uniform scores, warning ✓
|
||||||
- R-46: **this document** — GO; Bobenko+Springborn 2007 at rank 7 validates hub detection; score flatness expected at corpus size 29; increase to > 100 papers for stronger PageRank differentiation
|
- R-46: **this document** — GO; Bobenko+Springborn 2007 at rank 7 validates hub detection; score flatness expected at corpus size 29; increase to > 100 papers for stronger PageRank differentiation
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Audit Update (2026-06-15)
|
||||||
|
|
||||||
|
A post-hoc audit of the shipped F-15 code found that the live-corpus spike above
|
||||||
|
was measured on a graph the current code no longer produces:
|
||||||
|
|
||||||
|
- **ID-format mismatch resolved.** `build_citation_graph` resolves OpenAlex
|
||||||
|
`cited_id`s to the canonical `papers.id` through the shared
|
||||||
|
`RESOLVED_CITATIONS_SQL`, now also used by `codex.discover` so the two cannot
|
||||||
|
diverge (audit C-1). Ingest stores the caller's canonical id as `papers.id`
|
||||||
|
instead of OpenAlex's URL-form doi (audit C-7). Inter-KB citations are
|
||||||
|
therefore represented as edges — the "Known Limitation" above is obsolete.
|
||||||
|
- **Spike numbers are stale.** The 478 dangling nodes, the rank-7
|
||||||
|
Bobenko/Springborn result, and the 5.7 % score spread were measured *before*
|
||||||
|
the resolution + canonicalisation. They must be **re-measured after the corpus
|
||||||
|
is re-ingested** onto canonical ids (the chosen C-7 migration); until then the
|
||||||
|
GO rests on a superseded topology.
|
||||||
|
|
||||||
|
**Action (pending):** after re-ingesting via `ingest_all.sh`, re-run
|
||||||
|
`codex graph report`, refresh the Spike Result table, and confirm the
|
||||||
|
foundational hubs (Pinkall/Polthier, Yamabe, Schoen, Bobenko/Springborn) still
|
||||||
|
surface.
|
||||||
|
|||||||
139
infra/reingest_canonical_ids.sh
Executable file
139
infra/reingest_canonical_ids.sh
Executable file
@@ -0,0 +1,139 @@
|
|||||||
|
#!/usr/bin/env bash
|
||||||
|
# One-time migration to canonical paper IDs (audit C-7).
|
||||||
|
#
|
||||||
|
# Before the C-7 fix, ingest stored OpenAlex's URL-form doi/id as papers.id
|
||||||
|
# (e.g. "https://doi.org/10.48550/arxiv.math/0603097"). After the fix, ingest
|
||||||
|
# keys papers.id on the bare caller id ("math/0603097"). The existing rows keep
|
||||||
|
# their old URL-form primary keys, so a plain re-ingest would DUPLICATE every
|
||||||
|
# paper (new bare-id row alongside the old URL-id row). This script wipes the
|
||||||
|
# corpus and rebuilds it on canonical ids.
|
||||||
|
#
|
||||||
|
# DESTRUCTIVE: `TRUNCATE papers CASCADE` removes papers + chunks + citations +
|
||||||
|
# formulas + figures + paper_identifiers (code_links.paper_id is set NULL). All
|
||||||
|
# content is rebuilt by ingest_all.sh, which re-fetches OpenAlex and re-embeds
|
||||||
|
# (minutes for ~36 papers).
|
||||||
|
#
|
||||||
|
# Prerequisites: SSH tunnel on :5433 (see ingest_all.sh) and .env.jetson-ingest.
|
||||||
|
# Usage: bash infra/reingest_canonical_ids.sh
|
||||||
|
set -euo pipefail
|
||||||
|
|
||||||
|
CODEX_DIR="$(cd "$(dirname "$0")/.." && pwd)"
|
||||||
|
ENV_FILE="$CODEX_DIR/.env.jetson-ingest"
|
||||||
|
PY="$CODEX_DIR/.venv/bin/python"
|
||||||
|
TUNNEL_PORT=5433
|
||||||
|
|
||||||
|
# ── 1. Preconditions ─────────────────────────────────────────────────────────
|
||||||
|
if ! nc -z localhost "$TUNNEL_PORT" 2>/dev/null; then
|
||||||
|
echo "ERROR: SSH tunnel not active on :$TUNNEL_PORT"
|
||||||
|
echo "Run: ssh -f -N -L 5433:localhost:5432 alfred@192.168.178.103"
|
||||||
|
exit 1
|
||||||
|
fi
|
||||||
|
[[ -f "$ENV_FILE" ]] || { echo "ERROR: $ENV_FILE missing"; exit 1; }
|
||||||
|
[[ -x "$PY" ]] || { echo "ERROR: venv python not found at $PY (run uv sync)"; exit 1; }
|
||||||
|
# Load DATABASE_URL into the environment (not echoed — keeps the password out of logs).
|
||||||
|
set -a; source "$ENV_FILE"; set +a
|
||||||
|
|
||||||
|
# run_sql <SQL>: execute one statement via psycopg, print any result rows.
|
||||||
|
# Reads DATABASE_URL from the environment; suppresses the DSN on error.
|
||||||
|
run_sql() {
|
||||||
|
"$PY" - "$1" <<'PYEOF'
|
||||||
|
import os, sys, psycopg
|
||||||
|
from psycopg.rows import dict_row
|
||||||
|
try:
|
||||||
|
with psycopg.connect(os.environ["DATABASE_URL"], row_factory=dict_row, connect_timeout=10) as c:
|
||||||
|
cur = c.execute(sys.argv[1])
|
||||||
|
if cur.description:
|
||||||
|
for row in cur.fetchall():
|
||||||
|
print(" " + " ".join(f"{k}={v!r}" for k, v in row.items()))
|
||||||
|
c.commit()
|
||||||
|
except Exception as e:
|
||||||
|
print(f" DB ERROR: {type(e).__name__} (details suppressed to avoid DSN leak)")
|
||||||
|
sys.exit(1)
|
||||||
|
PYEOF
|
||||||
|
}
|
||||||
|
|
||||||
|
# ── 2. BEFORE snapshot ───────────────────────────────────────────────────────
|
||||||
|
echo "── BEFORE migration ──────────────────────────────────────────"
|
||||||
|
run_sql "SELECT count(*) AS papers, count(*) FILTER (WHERE starts_with(id, 'https://')) AS url_form_ids FROM papers"
|
||||||
|
echo " sample ids:"
|
||||||
|
run_sql "SELECT id FROM papers ORDER BY added_at LIMIT 3"
|
||||||
|
|
||||||
|
# ── 2b. Preflight: verify the schema is re-ingestable BEFORE wiping anything ──
|
||||||
|
# ingest writes chunks.section (F-16). On a live DB that predates it the column
|
||||||
|
# is missing, and the app user cannot ALTER tables owned by 'postgres' (audit
|
||||||
|
# M-1). Check FIRST so we never TRUNCATE and then fail mid-re-ingest.
|
||||||
|
echo "── Preflight: schema readiness ───────────────────────────────"
|
||||||
|
SECTION_OK="$("$PY" - <<'PYEOF'
|
||||||
|
import os, psycopg
|
||||||
|
try:
|
||||||
|
with psycopg.connect(os.environ["DATABASE_URL"], connect_timeout=10) as c:
|
||||||
|
cols = [r[0] for r in c.execute(
|
||||||
|
"SELECT column_name FROM information_schema.columns WHERE table_name='chunks'").fetchall()]
|
||||||
|
print("yes" if "section" in cols else "no")
|
||||||
|
except Exception:
|
||||||
|
print("error")
|
||||||
|
PYEOF
|
||||||
|
)"
|
||||||
|
if [[ "$SECTION_OK" != "yes" ]]; then
|
||||||
|
echo "ABORT — schema not ready: chunks.section is missing (audit M-1)."
|
||||||
|
echo "The app user cannot add it (chunks is owned by 'postgres'). Apply it as"
|
||||||
|
echo "the table owner, then re-run this script:"
|
||||||
|
echo ""
|
||||||
|
echo " ssh alfred@192.168.178.103 \\"
|
||||||
|
echo " \"sudo -u postgres psql -d papers -c \\\\\"ALTER TABLE chunks ADD COLUMN IF NOT EXISTS section TEXT;\\\\\"\""
|
||||||
|
echo ""
|
||||||
|
echo "Nothing was changed — no TRUNCATE was issued."
|
||||||
|
exit 1
|
||||||
|
fi
|
||||||
|
echo " chunks.section present — schema OK."
|
||||||
|
|
||||||
|
# ── 3. Confirmation gate (destructive) ───────────────────────────────────────
|
||||||
|
echo ""
|
||||||
|
echo "This TRUNCATEs papers CASCADE (papers/chunks/citations/formulas/figures/"
|
||||||
|
echo "paper_identifiers) and re-ingests via ingest_all.sh. The DB content is"
|
||||||
|
echo "rebuilt from scratch."
|
||||||
|
read -r -p "Type 'MIGRATE' to proceed: " confirm
|
||||||
|
[[ "$confirm" == "MIGRATE" ]] || { echo "Aborted — nothing changed."; exit 1; }
|
||||||
|
|
||||||
|
# ── 4. Wipe ──────────────────────────────────────────────────────────────────
|
||||||
|
echo "── TRUNCATE papers CASCADE ───────────────────────────────────"
|
||||||
|
run_sql "TRUNCATE papers CASCADE"
|
||||||
|
echo " wiped."
|
||||||
|
|
||||||
|
# ── 5. Re-ingest on canonical ids ────────────────────────────────────────────
|
||||||
|
echo "── Re-ingest (ingest_all.sh) ─────────────────────────────────"
|
||||||
|
bash "$CODEX_DIR/ingest_all.sh"
|
||||||
|
|
||||||
|
# ── 6. AFTER snapshot + live verification ────────────────────────────────────
|
||||||
|
echo "── AFTER migration ───────────────────────────────────────────"
|
||||||
|
echo " C-7 — url_form_ids should now be 0; sample ids should be bare:"
|
||||||
|
run_sql "SELECT count(*) AS papers, count(*) FILTER (WHERE starts_with(id, 'https://')) AS url_form_ids FROM papers"
|
||||||
|
run_sql "SELECT id FROM papers ORDER BY added_at LIMIT 3"
|
||||||
|
|
||||||
|
# C-1 must be verified through the REAL resolver-based discovery_leads(), not a
|
||||||
|
# raw cited_id check: cited_id/openalex_id stay in OpenAlex form, so the raw
|
||||||
|
# "cited_id NOT IN papers.id" count is non-zero by design — the resolver is what
|
||||||
|
# excludes ingested papers. Check that no ingested paper leaks into the leads.
|
||||||
|
echo " C-1 — real discovery_leads() must contain no already-ingested paper:"
|
||||||
|
PYTHONPATH="$CODEX_DIR" "$PY" - <<'PYEOF'
|
||||||
|
import os, psycopg
|
||||||
|
from psycopg.rows import dict_row
|
||||||
|
from codex.discover import discovery_leads
|
||||||
|
try:
|
||||||
|
leads = discovery_leads(limit=100000)
|
||||||
|
with psycopg.connect(os.environ["DATABASE_URL"], row_factory=dict_row, connect_timeout=10) as c:
|
||||||
|
ingested = {r["id"] for r in c.execute("SELECT id FROM papers").fetchall()}
|
||||||
|
ingested |= {
|
||||||
|
r["openalex_id"]
|
||||||
|
for r in c.execute("SELECT openalex_id FROM papers WHERE openalex_id IS NOT NULL").fetchall()
|
||||||
|
}
|
||||||
|
leaked = sum(1 for lead in leads if lead["cited_id"] in ingested)
|
||||||
|
print(f" leads={len(leads)} ingested-papers-leaked-into-leads={leaked}")
|
||||||
|
except Exception as e:
|
||||||
|
print(f" DB ERROR: {type(e).__name__} (details suppressed)")
|
||||||
|
PYEOF
|
||||||
|
|
||||||
|
echo ""
|
||||||
|
echo "✓ Migration complete."
|
||||||
|
echo " Expected: url_form_ids=0 (C-7 verified) and leaked=0 (C-1 verified) above."
|
||||||
|
echo " Next: re-run 'codex graph report' and refresh the ADR-F15 spike table (audit D-1)."
|
||||||
@@ -106,6 +106,37 @@ def test_ingest_paper_basic() -> None:
|
|||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_ingest_canonicalises_paper_id_to_caller_arg() -> None:
|
||||||
|
"""papers.id is the caller-supplied id, not OpenAlex's URL-form id (audit C-7).
|
||||||
|
|
||||||
|
Real OpenAlex returns the doi/id as a full URL; ingest must key the paper on
|
||||||
|
the bare id the caller passed (matching ingest scripts and CLI lookups), and
|
||||||
|
keep the OpenAlex work id only as openalex_id.
|
||||||
|
"""
|
||||||
|
oa_paper = Paper(
|
||||||
|
id="https://doi.org/10.48550/arxiv.math/0603097", # URL form, as OpenAlex emits
|
||||||
|
title="A Test Paper",
|
||||||
|
openalex_id="https://openalex.org/W4301005794",
|
||||||
|
authors=["Alice", "Bob"],
|
||||||
|
year=2008,
|
||||||
|
abstract="Test abstract.",
|
||||||
|
)
|
||||||
|
mock_conn = MagicMock()
|
||||||
|
|
||||||
|
with (
|
||||||
|
patch("codex.ingest.openalex.fetch_paper", return_value=oa_paper),
|
||||||
|
patch("codex.ingest.openalex.fetch_citations", return_value=[]),
|
||||||
|
patch("codex.ingest.get_embedder", return_value=_fake_embedder()),
|
||||||
|
patch("codex.ingest.get_conn", side_effect=_make_conn_cm(mock_conn)),
|
||||||
|
):
|
||||||
|
result = ingest_paper("math/0603097")
|
||||||
|
|
||||||
|
papers_params = mock_conn.execute.call_args_list[0][0][1]
|
||||||
|
assert papers_params["id"] == "math/0603097", "papers.id must be the caller's bare id"
|
||||||
|
assert papers_params["openalex_id"] == "https://openalex.org/W4301005794"
|
||||||
|
assert result.paper_id == "math/0603097"
|
||||||
|
|
||||||
|
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
# 2. test_ingest_paper_source_tex
|
# 2. test_ingest_paper_source_tex
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
@@ -320,9 +351,11 @@ def test_ingest_paper_rich_calls_formula_and_figure_parsers(tmp_path: Any) -> No
|
|||||||
mock_conn.executemany = MagicMock()
|
mock_conn.executemany = MagicMock()
|
||||||
mock_conn.commit = MagicMock()
|
mock_conn.commit = MagicMock()
|
||||||
|
|
||||||
fake_formula = FormulaChunk(paper_id=paper.id, page=1, raw_latex=r"\alpha", context="test")
|
# Extractors derive paper_id from the PDF filename stem ("paper"), which differs
|
||||||
|
# from papers.id — ingest must insert papers.id, not this stem (audit C-10).
|
||||||
|
fake_formula = FormulaChunk(paper_id="paper", page=1, raw_latex=r"\alpha", context="test")
|
||||||
fake_figure = FigureChunk(
|
fake_figure = FigureChunk(
|
||||||
paper_id=paper.id, page=1, image_path="/tmp/fig.png", caption="Figure 1."
|
paper_id="paper", page=1, image_path="/tmp/fig.png", caption="Figure 1."
|
||||||
)
|
)
|
||||||
|
|
||||||
mock_settings = MagicMock()
|
mock_settings = MagicMock()
|
||||||
@@ -349,6 +382,16 @@ def test_ingest_paper_rich_calls_formula_and_figure_parsers(tmp_path: Any) -> No
|
|||||||
assert result.formulas_upserted == 1
|
assert result.formulas_upserted == 1
|
||||||
assert result.figures_upserted == 1
|
assert result.figures_upserted == 1
|
||||||
|
|
||||||
|
# C-10: formula/figure rows must be keyed on papers.id, not the extractor's
|
||||||
|
# filename-stem paper_id, or the FK to papers(id) is violated.
|
||||||
|
cursor = mock_conn.cursor.return_value.__enter__.return_value
|
||||||
|
inserted_paper_ids = {
|
||||||
|
row[0] for call in cursor.executemany.call_args_list for row in call[0][1]
|
||||||
|
}
|
||||||
|
assert inserted_paper_ids == {paper.id}, (
|
||||||
|
f"formula/figure inserts must use papers.id, got {inserted_paper_ids}"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
def test_ingest_paper_no_rich_skips_parsers(tmp_path: Any) -> None:
|
def test_ingest_paper_no_rich_skips_parsers(tmp_path: Any) -> None:
|
||||||
"""When rich=False (default), formula and figure parsers are NOT called."""
|
"""When rich=False (default), formula and figure parsers are NOT called."""
|
||||||
|
|||||||
@@ -15,7 +15,8 @@ from codex.sources.openalex import _resolve_id
|
|||||||
|
|
||||||
_SAMPLE_WORK = {
|
_SAMPLE_WORK = {
|
||||||
"id": "https://openalex.org/W2741809807",
|
"id": "https://openalex.org/W2741809807",
|
||||||
"doi": "10.1145/3592430",
|
# Real OpenAlex returns the DOI as a full URL, not a bare DOI (audit T-3).
|
||||||
|
"doi": "https://doi.org/10.1145/3592430",
|
||||||
"title": "Conformal Prediction: A Review",
|
"title": "Conformal Prediction: A Review",
|
||||||
"publication_year": 2023,
|
"publication_year": 2023,
|
||||||
"authorships": [
|
"authorships": [
|
||||||
@@ -90,6 +91,10 @@ def test_fetch_paper_success(monkeypatch: pytest.MonkeyPatch) -> None:
|
|||||||
assert paper.authors == ["Alice Smith", "Bob Jones"]
|
assert paper.authors == ["Alice Smith", "Bob Jones"]
|
||||||
assert paper.abstract == "Conformal prediction is great"
|
assert paper.abstract == "Conformal prediction is great"
|
||||||
assert paper.openalex_id == "https://openalex.org/W2741809807"
|
assert paper.openalex_id == "https://openalex.org/W2741809807"
|
||||||
|
# fetch_paper maps id from the (URL-form) doi; ingest later canonicalises
|
||||||
|
# papers.id to the caller's bare id (audit C-7). Pin the real mapping here so
|
||||||
|
# the fixture cannot silently misrepresent OpenAlex again (audit T-3).
|
||||||
|
assert paper.id == "https://doi.org/10.1145/3592430"
|
||||||
|
|
||||||
|
|
||||||
def test_fetch_paper_arxiv_id_prefixed(monkeypatch: pytest.MonkeyPatch) -> None:
|
def test_fetch_paper_arxiv_id_prefixed(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||||
|
|||||||
Reference in New Issue
Block a user