diff --git a/docs/audit/AUDIT-2026-06-15-loop-1.md b/docs/audit/AUDIT-2026-06-15-loop-1.md new file mode 100644 index 0000000..1704b55 --- /dev/null +++ b/docs/audit/AUDIT-2026-06-15-loop-1.md @@ -0,0 +1,227 @@ +# AUDIT — Development Loop 1 (main branch) + +**Auditor:** Audit-Loop (Opus) +**Date:** 2026-06-15 +**Scope:** entire `main` branch (HEAD `5dfa6e4`) +**Method:** static review of source + schema + tests, branch-wide pattern scan +(`ruff`, `mypy`, defect-class greps), full test run (`330 passed`). + +> Status legend — **HIGH** = fix before relying on the feature · +> **MED** = fix soon, correctness/maintainability · **LOW** = polish · +> **INFO** = accepted risk / no action, documented for the record. + +This document **only records findings**. Remediation planning is a separate +step (next phase of the audit loop). + +--- + +## 0. Headline + +The most recent dev-loop work (**F-15 citation graph** + the follow-up fixes +`cited_id→papers.id` resolution and the `paper_identifiers` alias table) is +**functionally green but unverified where it matters**: the SQL that *is* the +fix has zero real-database coverage, the live-corpus validation in the ADR +predates the fix, and the same ID-mismatch bug the fix solves still lives in a +parallel code path (`discover.py`). Separately, a **live DB password sits in an +un-ignored file** and the new table has **no migration path onto the live DB**. + +Test suite: **330 passed**, `ruff check` clean, `ruff format --check` clean, +`mypy` clean on reviewed modules. + +--- + +## 1. Security + +### S-1 — Live DB credential in an un-ignored file · **HIGH** +- **Evidence:** `.env.jetson-ingest` contains + `DATABASE_URL=postgresql://researcher:05761b87…@localhost:5433/papers`. + `.gitignore` ignores only the literal `.env` (line: `# Environment secrets — NEVER commit` → `.env`). + `git check-ignore .env.jetson-ingest` → **NOT IGNORED**. +- **Impact:** one `git add -A && git commit` leaks a real Postgres password to + history. The file is currently untracked (`??`), so the leak has **not** + happened yet — this is a near-miss, not an incident. +- **Note:** the same gap exposes any future `.env.` file. + +### S-2 — Non-constant-time token comparison (MCP HTTP) · **LOW** +- **Evidence:** `codex/mcp_server.py:340` — `if auth != f"Bearer {token}":`. +- **Impact:** theoretical timing side-channel on the Bearer token. Low on a + trusted LAN, but trivially fixable with `secrets.compare_digest`. + +### S-3 — DNS-rebinding protection disabled · **INFO (accepted)** +- **Evidence:** `codex/mcp_server.py:35-37`, + `TransportSecuritySettings(enable_dns_rebinding_protection=False)` (commit `d29dcf2`). +- **Assessment:** documented and mitigated — HTTP transport requires a Bearer + token (`_TokenAuth`) and is firewalled to LAN IPs (UFW). A rebound browser + request lacks the token. Residual risk only if the firewall assumption fails. + No action; recorded so the trade-off stays visible. + +--- + +## 2. Correctness + +### C-1 — `discover.py` still has the ID-format-mismatch bug F-15 fixed · **HIGH** +- **Evidence:** `codex/discover.py:20` — + `WHERE cited_id NOT IN (SELECT id FROM papers)`; same pattern in + `cocited_papers` (`discover.py:52-61`), `citing_papers`, `cited_by`. +- **Root cause:** `citations.cited_id` is a **mix** of OpenAlex W-IDs (OpenAlex + API path, `ingest.py:229-231` ← `sources/openalex.py:155`) and DOIs/arXiv + (GROBID/S2 paths). `papers.id` is the canonical DOI/arXiv. An ingested paper + cited by its **OpenAlex ID** is *not* in `(SELECT id FROM papers)`, so it is + wrongly reported as a discovery lead. +- **Impact:** `codex discover leads`, `codex discover cocited`, and the MCP + `discover_leads` tool over-report — foundational papers the user has **already + ingested** show up as "cited but not ingested". This is exactly the + D-05-class issue the F-15 graph path resolves via the + `papers.openalex_id` JOIN (`graph.py:48-56`) — but the fix was **not + propagated** to `discover.py`. Two divergent "dangling" implementations now + disagree on what "ingested" means. + +### C-2 — `paper_identifiers` JOIN is effectively inert · **MED** +- **Evidence:** `graph.py:50-56` second LEFT JOIN; populated only at + `ingest.py:153-161`, which inserts **the same** `paper.openalex_id` that + `papers.openalex_id` already holds. +- **Impact:** because ingest never records an alias that differs from + `papers.openalex_id`, the `pi` JOIN can only resolve what the `p` JOIN already + resolves. It adds value **only** in the narrow case where a paper's + `openalex_id` changes across re-ingests (old alias retained via + `ON CONFLICT DO NOTHING`). The table + unique index + seed INSERT + migration + burden buy near-zero behaviour today — speculative generality. No test + exercises an alias that diverges from `papers.openalex_id`. + +### C-3 — MCP `wiki_read` returns the slug as its own "sources" · **MED** +- **Evidence:** `codex/mcp_server.py:138-149` — comment says "Collect cited + bibkeys", code sets `sources = [concept_slug]`. +- **Impact:** the `sources` field is a placeholder masquerading as data; any MCP + client trusting it to list cited bibkeys gets the concept's own slug. + +--- + +## 3. Documentation / Code Drift + +### D-1 — ADR-F15 validation predates the shipped graph · **HIGH (integrity)** +- **Evidence:** ADR-F15 (commit `a1a6f45`) lands **before** the resolution fixes + `664906f` (`cited_id→papers.id`) and `5dfa6e4` (`paper_identifiers`). +- **Stale claims:** + - "Spike Result": *478 dangling nodes*, specific PageRank scores, + *Bobenko/Springborn rank 7*, *5.7 % spread* — all measured on the + **pre-resolution** (bipartite DOI→OpenAlex) graph. Post-fix, in-KB cited + papers resolve to DOIs, so dangling count and PageRank distribution change. + - "Decision → Graph Construction": "inter-KB citations are structurally + invisible" — **now false**. + - "Known Limitation: ID-Format Mismatch": "cross-citations… not represented" + — **now resolved by the code**, contradicting the section. +- **Impact:** the **GO** decision rests on numbers the shipping code no longer + produces. Validation must be re-run on the post-fix graph (or the ADR marked + validation-pending). + +### D-2 — `graph_cite_boost_alpha` doc describes the *fixed* bug · **MED** +- **Evidence:** `config.py:296-300` describes + `Final score = dense_score * (1 + alpha * pagerank_score)` — a **multiply**. + Actual code (`cli.py:107`) divides distance: + `boosted = distance / (1.0 + alpha * pr_score)`. The ADR explicitly states the + multiply form was the "critical bug corrected during the F-15 review gate". +- **Impact:** the config docstring still documents the inverted (buggy) formula. + +### D-3 — `paper_identifiers` tagged "F-17" in schema, shipped as F-15 fix · **LOW** +- **Evidence:** `infra/schema.sql:129` comment "F-17 Paper identifier aliases", + but the table is introduced by the F-15 graph follow-up (`5dfa6e4`) and the + ADR-F15 work. Feature-tag inconsistency; ADR-F15 doesn't mention the table at all. + +--- + +## 4. Schema / Migration + +### M-1 — New table has no migration path onto the live DB · **HIGH (operational)** +- **Evidence chain:** + 1. `apply_schema` docstring (`db.py:46-49`) claims "all DDL statements use + CREATE … IF NOT EXISTS" — **false**: `papers`, `chunks`, `citations`, + `code_links` and their indexes use bare `CREATE TABLE/INDEX` + (`schema.sql:16,33,56,68,30,41,…`). Only F-09/F-16/F-17 DDL is idempotent. + 2. `apply_schema` is **never called** anywhere in the app — no `init`/`migrate` + CLI command (`grep apply_schema` → only `db.py` + a mocked unit test). + 3. `ingest_all.sh` does **not** apply the schema; it only runs `codex ingest`. +- **Impact:** re-running `infra/schema.sql` against the **existing** Jetson DB + fails on the first `CREATE TABLE papers` (already exists) and rolls back the + whole script in one implicit transaction — so the new `paper_identifiers` + table (and its seed INSERT) is **never created** on a DB that predates it. + `build_citation_graph` then fails with `relation "paper_identifiers" does not + exist` on the live corpus. The migration step is currently manual and undocumented. + +--- + +## 5. Test Integrity + +### T-1 — The F-15 resolution SQL has zero real coverage · **HIGH** +- **Evidence:** every test in `tests/graph/test_graph.py` and + `tests/graph/test_cli.py` builds the graph from a `MagicMock` conn + (`_make_conn`, `_make_conn_with_paper_ids`). `test_cross_citation_uses_doi_when_in_kb` + (`test_graph.py:83-99`) **pre-resolves** the rows in Python and asserts the + graph builds — it tests the mock, not the `COALESCE(p.id, pi.paper_id, …)` + + two LEFT JOINs that *are* the fix. +- **Impact:** the substance of the two most recent commits is unverified. 330 + green tests give false confidence about the resolution logic, the `p.id IS NULL` + correlated guard, and the `paper_identifiers` unique-index behaviour. Needs an + integration test against a real (or `pytest`-managed) Postgres. + +### T-2 — `apply_schema` idempotency untested · **MED** +- **Evidence:** `tests/scaffold/test_db.py:24-32` mocks the conn and only checks + `execute`/`commit` are called. The (false) idempotency claim in M-1 is never + exercised. + +--- + +## 6. Design / Usability + +### U-1 — `graph report` prints bare IDs, no titles · **LOW** +- `cli.py:582-588` emits `score ` and bare dangling IDs — a mix of DOIs and + `W…` OpenAlex IDs with no title/year. The ADR's readable author/title table is + not what the CLI produces; output is hard to interpret. + +### U-2 — `--cite-boost` can only reorder within the top-`limit` page · **LOW** +- `cli.py:84-94` applies `ORDER BY … LIMIT limit` in SQL **before** the boost + (`cli.py:96-109`). A high-PR paper just outside the semantic cut can never be + pulled in. With α=0.3 and PR≈0.002 the effect is ~0.06 % — effectively a + within-page tie-breaker. Consistent with ADR intent, but worth stating as a + limitation (the flag does far less than "weight results by PageRank" implies). + +### U-3 — `graph report --json` drops the small-corpus warning · **LOW** +- `cli.py:557-570` returns before the `graph_min_corpus_size` warning + (`cli.py:572-577`). JSON consumers get neither the warning nor a flag for it. + +### U-4 — `graph.find_co_cited` implemented + tested but not wired to a CLI · **LOW** +- `graph.find_co_cited` (`graph.py:142`) is unit-tested but exposed by no `graph` + sub-command (only the separate SQL-based `discover cocited` is wired). ADR-F15 + R-43's co-citation is only half-surfaced. + +--- + +## 7. State of Tree (INFO — no defect) + +- The uncommitted diff (`wiki.py`, `synthesis.py`, `semanticscholar.py`, + `tests/synthesis/test_cli.py`, `tests/wiki/test_compile.py`) is a **legitimate + `ruff format` normalization** to `line-length=100`. `ruff format --check` + reports all 25 files formatted *with* these changes applied, meaning the + **committed** code was non-compliant. These edits should be **committed**, not + reverted. (Not a finding; clarifies the working-tree churn.) + +--- + +## 8. Coverage of this audit (honesty ledger) + +**Deep-reviewed:** `graph.py`, `db.py`, `infra/schema.sql`, `ingest.py`, +`discover.py`, `provenance.py` (scan), `mcp_server.py`, `cli.py` (search + +graph), `config.py` (graph settings), F-15 tests, ADR-F15; branch-wide pattern +scan (SQL injection, bare except, eval/exec/subprocess, secrets, timeouts). + +**Not yet deep-reviewed (no findings asserted):** +- `synthesis.py` grounding guard / claim parser (`_run_grounding_guard`, + `_parse_claims`) — the "zero-fact-leak (N-02)" core. Prompts reviewed; guard + logic not yet traced. +- `wiki.py` grounding guard + conflict detection (only the format diff seen). +- `sources/` (arxiv, openalex, semanticscholar) beyond citation shapes. +- `parsing/` (grobid, nougat, mathpix, figures, tex) — error handling in + `mathpix.py`/`figures.py` uses broad `except Exception` without `noqa` + (`mathpix.py:109,151`, `figures.py:125,152`) — flagged for a later pass. +- `embed.py`, `quality.py`, `models.py`. + +A second pass should close these before the audit is declared complete.