docs(audit): record development-loop-1 findings
Audit of main @5dfa6e4: 4 HIGH (S-1 secret-in-tree, M-1 missing migration path, C-1 unpropagated ID-mismatch fix in discover.py, D-1 stale ADR-F15 validation, T-1 untested resolution SQL) plus MED/LOW findings. Findings only; remediation planning deferred to the next loop phase. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Tarik Moussa
227
docs/audit/AUDIT-2026-06-15-loop-1.md
Normal file
227
docs/audit/AUDIT-2026-06-15-loop-1.md
Normal file
@@ -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.<profile>` 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 <id>` 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.
|
||||
Reference in New Issue
Block a user