Files
codex-py/docs/audit/AUDIT-2026-06-15-loop-1.md
Tarik Moussa a0ad069b1d 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>
2026-06-15 08:43:54 +02:00

12 KiB
Raw Permalink Blame History

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-ingestNOT 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:340if 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:20WHERE 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-231sources/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.