refactor(import-metadata): sidecar-only; CSV loads via sync-metadata (SA-teardown-A)

[mousebrains]

What

Dataset-separation SA-teardown, part A (the load-path slice). Makes scripts/import_metadata.py a sidecar-only applier and routes all CSV metadata loads through levels sync-metadata.

Why

levels sync-metadata already applies the CSV columns by stable id with delete-safety (#146). reach.geom/reach.gradient_profile are EXCLUDED_COLUMNS — applied only from reaches.json/reaches-gradient.json. So import_metadata.py's full-CSV upsert was a redundant parallel-to-sync-metadata channel that skipped #146's delete-safety (the #148 review's "optional hardening"). This removes that channel entirely (not just gates it) and keeps the one thing only import_metadata can do: apply the geometry sidecars.

This is the master-plan's "import_metadata → wrapper / standardize on sync-metadata", reconciled with the reality that sync-metadata can't apply the geom/gradient sidecars.

Change

• import_metadata.py — drop the full-CSV upsert path (+ the now-unused metadata_csv import). No flags = apply both sidecars; --geom-only/--gradient-only = one. Still the sanctioned sidecar applier (no refuse_configured_db, per the SA-3 safety.py enumeration). deploy.sh 3.25/3.26 (--geom-only/--gradient-only) unchanged; 3.1 already does CSV via sync-metadata.
• Fresh-load / recovery / onboarding runbooks now run init-db --no-seed → sync-metadata → import_metadata (sidecars) → pipeline: CLAUDE.md, README.md, CONTRIBUTING.md, deploy/SETUP.md (§4 + the second install block), and docs/migrations.md's recovery runbook (incl. the --no-seed-rationale paragraph — the collision + FK semantics are sync-metadata's now).
• Docstring sweep: safety.py (import_metadata has no full-CSV mode) + metadata_csv.py (sole upsert caller is now sync-metadata).
• Tests: the full-CSV round-trip tests apply the CSV half via sync_metadata (new _sync_csvs helper writes the minimal contract); the geom/gradient sidecar tests are unchanged; the two import-CSV-specific tests renamed.

Verification

• Real-DB fresh-load on a copy of the live DB: init-db --no-seed → sync-metadata --csv-dir <kayak_data> → import_metadata reproduces prod exactly — 328 sources / 231 gauges / 328 gauge_source / 432 reaches, incl. all 432 geoms + 432 gradients — and build renders. Proves sync-metadata is a complete CSV load path from empty.
• Full gate green: ruff (rc 0), ruff format --check (rc 0), mypy, pytest -m "not slow" = 1402 passed, wheel-smoke, git diff --check clean.
• Two-round adversarial self-review: round 1 confirmed the runtime change correct but caught stale recovery/onboarding docs (CONTRIBUTING.md, docs/migrations.md recovery runbook) still prescribing the removed no-flag CSV load — fixed; round 2 caught one more paragraph in the same runbook and verified the rest of the sweep is complete.

Scope / next

No schema change; no kayak_data change. Next: SA-teardown-B removes the snapshot script + systemd units + hc_metadata_snapshot config + export_metadata→levels recover-metadata; then SA-teardown-C (operational) disables the prod timer and enables kayak_data branch protection.

🤖 Generated with Claude Code

[mousebrains]

Adversarial review findings:

1. P2 - setup runbooks still fail without DATASET_DIR. CONTRIBUTING.md:5-19 and deploy/SETUP.md:737-760 run levels sync-metadata but never clone/configure kayak_data or set DATASET_DIR in those setup flows. In this PR worktree the default is still repo-relative data/db, and that directory no longer exists, so following the runbook fails at the metadata sync:

   error: csv dir does not exist: /private/tmp/kayak-pr149-review/data/db
   

   The README and the main deploy setup section have the right pattern. These two blocks need the same git clone ... kayak_data + DATASET_DIR=... step, or they should delegate explicitly to the README/deploy section before the sync command.

2. P2 - import_metadata.py still reports success when it applies zero sidecar rows. scripts/import_metadata.py:84-92, scripts/import_metadata.py:110-118, and scripts/import_metadata.py:197-200 only warn when sidecar ids do not match DB reach rows, then return 0 as long as integrity_check passes. After this PR removes the CSV-loading path from the script, that masks the most likely operator mistake: running the sidecar apply before levels sync-metadata, or against the wrong/empty DB. I verified an init-db --no-seed DB followed by no-flag python scripts/import_metadata.py exits 0 while applying 0/432 reaches.json rows and 0/432 gradient rows. Under set -e, that looks like a successful apply. I would make unmatched sidecar ids a non-zero exit, or require an explicit --allow-missing-reaches escape hatch if partial applies are intentionally supported.

What I verified:

• gh pr checks 149: all checks passing.
• git diff --check 5502ba841b2275759ce50f32cac868a5442ead43...HEAD: clean.
• env UV_CACHE_DIR=/private/tmp/kayak-pr149-uv-cache uv run --extra dev pytest -q tests/test_scripts/test_metadata_roundtrip.py tests/test_cli/test_sync_metadata.py tests/test_dataset_contract.py tests/test_scripts/test_validate_dataset.py: 181 passed.
• levels validate-dataset /Users/pat/tpw/kayak_data: OK against the local clean kayak_data checkout.
• Fresh rebuild smoke using the live dataset: init-db --no-seed -> levels sync-metadata -> no-flag scripts/import_metadata.py succeeded, applied 432 geom rows and 432 gradient rows, then passed integrity_check, levels orphan-check, and levels check-reaches.
• Compared rebuilt sidecars against the fresh live DB copy from ../DB: all 432 reach geom values and all 432 gradient_profile values matched. The only metadata parity difference I saw was existing fetch_url.hours normalization (NULL from CSV sync vs '' in the live DB), which is semantically equivalent for current use and not introduced by this PR.

[mousebrains]

Adversarial review — PR #149 (SA-teardown-A: import_metadata sidecar-only; CSV loads via sync-metadata)

Reviewed on the live host. This implements the #148 review's "optional hardening" — removing import_metadata.py's full-CSV upsert (the parallel-to-sync-metadata channel that skipped #146's delete-safety) entirely, leaving it the sidecar-only applier. I traced the flag logic, audited every import_metadata.py invocation in the post-PR tree for the one real regression risk (a bare call that silently skips CSVs), and checked the test rework.

Verdict: ready to merge. The code change is small and correct, the runtime behavior I can verify locally is unchanged, and all runbooks/deploy paths are consistent. One claim rests on the author's reproduction rather than mine (see Findings); it's low-risk for deploy and I note where I'd keep it as an acceptance step.

What I verified

• Flag logic is correct and the sidecar apply is unchanged. I traced all four combinations of apply_geom = geom_only or not gradient_only / apply_gradient = gradient_only or not geom_only: no flags → both; --geom-only → geom; --gradient-only → gradient; both → both. This matches the old sidecar selection exactly (the prior if args.geom_only or not args.gradient_only pair). _apply_geom/_apply_gradient themselves changed only in a column-width format string (20→22). So the only behavioral removal is the CSV upsert — _load_csvs and the metadata_csv as mc import are gone, and nothing else references mc.
• No orphaned bare call silently skips CSVs. This was my primary concern: a bare import_metadata.py (which used to load CSV+sidecars, now sidecars-only) left in any runbook would produce an empty/CSV-less site. I grepped the whole post-PR tree and read every site's context — every bare call is now preceded by levels sync-metadata:
  • scripts/deploy.sh: 3.1 = levels sync-metadata --backup (CSVs); 3.25/3.26 = import_metadata.py --geom-only / --gradient-only (sidecars). No bare call. ✓
  • CLAUDE.md §Quick start, README.md, CONTRIBUTING.md, deploy/SETUP.md (§4 and the §6 second install block), docs/migrations.md recovery runbook — all now init-db --no-seed → sync-metadata → import_metadata → pipeline. ✓
  • The remaining scripts/import_metadata.py hits in CI/Makefile/pre-commit are mypy <file> (type-check targets, not invocations). ✓
• Test rework is sound. The full-CSV round-trip tests now apply the CSV half via sync_metadata() (new _sync_csvs helper writing a minimal dataset.yaml for the contract gate) + the sidecar via import_metadata. The renamed test_csv_apply_preserves_geom_absent_from_snapshot now genuinely exercises sync-metadata's EXCLUDED-column behavior (a live geom absent from the snapshot survives the CSV apply), and test_resync_idempotent_across_pk_shapes still hits the composite-PK DO NOTHING/DO UPDATE paths. test_geom_only_applies_geom_leaves_metadata is unchanged. Bare-run-applies-both is covered by the round-trip tests. Coverage is preserved, retargeted to the surviving path.
• Docstring sweep is accurate (safety.py now says import_metadata "carries no full-CSV mode"; metadata_csv.py says its sole upsert caller is sync-metadata and drops the now-false FK-OFF bullet).
• Live baseline matches the PR's reproduction target. I queried the live DB: source=328, gauge=231, gauge_source=328, reach=432, all 432 geoms + 432 gradients — exactly the numbers the PR claims to reproduce. So the dataset is complete and those are the real targets.

Findings

1. [Low / informational] The "sync-metadata is a complete CSV load path from empty" claim rests on the author's real-DB reproduction, not mine. I confirmed the live DB has the target rows, but I did not independently run the init-db --no-seed → sync-metadata empty→full load (I started to and stopped). The substantive behavioral difference from the removed path is FK enforcement: the old import_metadata ran foreign_keys=OFF (tolerant bulk load, orphan-tolerant); sync-metadata runs foreign_keys=ON. An empty→full load under FK-ON requires parent-before-child insert ordering (or no orphan FKs in the CSVs). The PR's stated reproduction (432 reaches + geoms loaded cleanly) is the evidence this is handled. Distinction that lowers risk: the deploy path (deploy.sh 3.1) is an incremental sync against the populated live DB — already proven across the #146/#147/#148 deploys — not a fresh load. The empty→full path matters only for disaster recovery and onboarding. Recommend keeping that empty→full reproduction as a documented acceptance step for this slice (it's the load-bearing guarantee for the recovery runbooks this PR rewrites).

2. [Low] Fresh-load runbooks assume a status: publishable dataset. sync-metadata refuses a status: scaffold dataset unless --allow-scaffold (exit 1). The rewritten runbooks run a plain levels sync-metadata, which is correct for the real (publishable) kayak_data but would dead-end an installer who cloned a scaffold dataset with a bare "refused" exit. Pre-existing dataset behavior, not introduced here — worth at most a one-line note in the recovery runbook. (No action required if all real datasets are publishable.)

3. [Nit] No CHANGELOG.md entry for a documented behavior change (import_metadata no longer loads CSVs). The existing CHANGELOG lines describing its upsert are now historically-superseded; a one-line entry would help an operator who reads CHANGELOG before a recovery.

Scope / deploy

No schema change, no migration, no kayak_data change. Same low-risk class as #148: a code-only pull where the runtime delta is confined to a dev/deploy maintenance script (import_metadata.py) — not in the hourly pipeline path. The doc sweep is the bulk of the diff and is internally consistent.

[mousebrains]

Follow-up: Finding #1 independently verified (empty→full load)

I reproduced the empty→full load myself on a scratch DB from the real kayak_data dataset (never touching the live DB), using this PR's sidecar-only import_metadata.py:

levels init-db --no-seed  →  levels sync-metadata  →  import_metadata.py

Result — byte-for-byte reproduces prod:

	source	gauge	gauge_source	reach	geom	gradient	reach_state	reach_class	reach_guidebook	fetch_url	calc
live	328	231	328	432	432	432	446	442	1126	115	22
scratch	328	231	328	432	432	432	446	442	1126	115	22

• sync-metadata loaded all 14 tables from empty as pure inserts (no deletes), and the resulting DB has 0 foreign_key_check violations + integrity_check = ok — so the FK-ON-vs-old-FK-OFF concern is moot: the dataset carries no orphan FKs and sync-metadata's load order is FK-clean from empty.
• The sidecar applier wrote exactly 432 geoms + 432 gradients; a sampled reach.geom blob is identical to live.

So Finding #1 is resolved — sync-metadata is a complete CSV load path from empty, independently confirmed, not just author-attested. No remaining concerns; the residuals (#2 scaffold-status assumption, #3 CHANGELOG) stay informational. Verdict unchanged: ready to merge.

[mousebrains]

Both P2s fixed in 49e78c4 (plus the Low/Nit notes). Thanks for the thorough probing — and for independently confirming the empty→full load reproduces prod byte-for-byte.

[P2] import_metadata.py reported success on a zero-row apply. Right — after removing the CSV path, a no-flag run against an empty/wrong DB (the "ran before sync-metadata" mistake) applied 0/432 + 0/432 and exited 0. Now _apply_geom/_apply_gradient return the count of snapshot ids that matched no reach row, and main() rolls back + exits non-zero when any are unmatched — with --allow-missing-reaches to opt into a deliberate partial apply. Verified your exact repro:

• empty DB, no flag → exit 1, error: 864 sidecar entr(ies) matched no reach row … Run levels sync-metadata first, nothing applied (rolled back).
• --allow-missing-reaches → exit 0 (partial).
• full DB (after sync-metadata) → exit 0.

Deploy 3.25/3.26 (--geom-only/--gradient-only against the full prod DB, after the 3.1 sync inserts any new reaches; validate-dataset's reach/snapshot-integrity check enforces reaches.json ids ⊆ reach.csv ids) all match, so this never false-positives on deploy. New test test_import_fails_loud_on_unmatched_sidecar_reach covers fail-loud + rollback + the flag.

[P2] Setup runbooks ran sync-metadata without DATASET_DIR. Fixed: CONTRIBUTING.md and deploy/SETUP.md's condensed install recap now clone kayak_data + set DATASET_DIR before the load (the recap's .env was missing it). (Pre-existing — import_metadata needed DATASET_DIR too — but A touches these blocks, so swept now.)

[Low] recovery runbook now notes sync-metadata refuses a status: scaffold dataset (--allow-scaffold to override; real kayak_data is publishable). [Nit] added a CHANGELOG entry for the import_metadata behavior change.

Full gate green: ruff/format (rc 0), mypy, pytest -m "not slow" = 1403, wheel-smoke, git diff --check clean. Still not merging — yours.

[mousebrains]

Adversarial rereview of 49e78c4:

1. P3 - deploy/SETUP.md local-dev recap still is not copy-paste safe from outside the repo. The condensed local-development setup now correctly clones kayak_data and writes DATASET_DIR, but step 6 still runs:

   /home/pat/.venv/bin/python scripts/import_metadata.py

   at deploy/SETUP.md:760-764 without first cding into /home/pat/kayak. The earlier main install section does cd /home/pat/kayak, but this local-dev block does not, and the surrounding commands use absolute paths (/home/pat/.venv/bin/levels, /home/pat/kayak[dev]), so it reads as self-contained. I reproduced the failure by running the same shape from /private/tmp:

   can't open file '/private/tmp/scripts/import_metadata.py': [Errno 2] No such file or directory
   

   Fix is small: add cd /home/pat/kayak before step 6, or make the script path absolute (/home/pat/kayak/scripts/import_metadata.py).

The two previous P2s look resolved:

• Empty/wrong DB sidecar apply now exits 1, prints the expected “Run levels sync-metadata first” error, and leaves the scratch DB unchanged.
• --allow-missing-reaches exits 0 as the explicit partial-apply escape hatch.
• Full scratch rebuild from the real kayak_data dataset still works: init-db --no-seed -> sync-metadata -> import_metadata.py applies 432 geoms + 432 gradients.
• A second identical import_metadata.py run also succeeds with 432/432, so the rowcount-based unmatched detection stays idempotent.

Verification run:

• gh pr checks 149: all checks passing on 49e78c4.
• git diff --check 5502ba841b2275759ce50f32cac868a5442ead43...HEAD: clean.
• env UV_CACHE_DIR=/private/tmp/kayak-pr149-uv-cache uv run --extra dev pytest -q tests/test_scripts/test_metadata_roundtrip.py tests/test_cli/test_sync_metadata.py tests/test_dataset_contract.py tests/test_scripts/test_validate_dataset.py: 182 passed.
• env UV_CACHE_DIR=/private/tmp/kayak-pr149-uv-cache uv run --extra dev mypy scripts/import_metadata.py: clean.
• levels validate-dataset /Users/pat/tpw/kayak_data: OK.
• Scratch rebuilt DB: integrity_check = ok, orphan-check clean, check-reaches checked 432 reaches with 0 issues.

[mousebrains]

Fixed the P3 in d5228ef: both deploy/SETUP.md blocks (the condensed local-dev recap step 6 and the §4 "apply geometry by hand" snippet) now use the absolute /home/pat/kayak/scripts/import_metadata.py, matching the absolute venv/levels paths around them — copy-paste-safe from any cwd. (Swept both occurrences, not just the recap.) Thanks for re-confirming the two P2s resolved + the idempotent re-run. Docs-only; git diff --check clean. Still not merging — yours.