fix(sync-metadata): refuse deletes all-or-nothing — no partial insert/update apply (SA)

[mousebrains]

What

Dataset-separation SA, first sub-slice. Makes levels sync-metadata all-or-nothing when a CSV diff contains deletions and --allow-deletes is not passed: it now refuses before any write (exit 2, zero commits), instead of committing the inserts/updates and only refusing the deletes.

This satisfies SA acceptance criterion #8: "Refused metadata deletes begin no write transaction and leave logical table checksums/counts unchanged."

Why

Today (src/kayak/cli/sync_metadata.py) a delete-containing diff without --allow-deletes still runs upsert_csvs() — committing every insert/update — and only refuses the deletes (exit 2). That leaves a half-applied DB: on a deploy that unexpectedly drops a row, the safe half is already committed when deploy.sh aborts.

Change

• Move the refusal ahead of the with conn: transaction. If plan.has_deletes and not args.allow_deletes → print the plan + per-source observation-drop counts (both already computed pre-transaction) and return 2 with no writes. The in-transaction delete branch is now reached only with --allow-deletes, so a recovery run applies the whole batch (inserts/updates AND deletes) atomically, and is idempotent.
• Deploy flow unchanged, strictly safer: deploy.sh step 3.1 still runs sync-metadata --backup without --allow-deletes; a delete still exits 2 and aborts the set -e deploy — but now the DB is untouched rather than half-applied. The operator's manual --allow-deletes run applies everything in one transaction, then re-runs deploy.

Tests

• test_deletes_refused_without_flag now asserts nothing applied, proven via PRAGMA data_version on a separate connection (bumps iff another connection commits) — a direct check that no write transaction committed.
• New test_refused_delete_then_allow_deletes_applies_whole_batch — the deploy recovery flow: refuse → byte-unchanged → --allow-deletes applies the whole batch → idempotent.
• New test_pure_upsert_without_allow_deletes_applies — a delete-free diff still applies without the flag (guards the common deploy + kayak_data CI path).
• Doc/comment sweep: module docstring, --allow-deletes help, deploy.sh step-3.1 comment, add-gauges runbook.

Verification

• Full local gate green: ruff check, ruff format --check, mypy (CI scope), pytest -m "not slow" = 1382 passed, wheel-smoke.
• Manual end-to-end repro on a scratch DB: refused pass leaves the would-be-renamed row unchanged (exit 2); --allow-deletes re-run applies rename + delete (exit 0); third run is a no-op.
• Two-round adversarial self-review (parallel finders + verify) — dry: no regressions, no stale docs, data_version assertion verified robust in both journal modes.

Scope / not in this PR

No schema, model, or migration change. kayak_data needs no change — its CI smoke (init-db --no-seed → sync-metadata ×2 → build) syncs into an empty DB (all inserts, zero deletes), so the gate never fires.

Next SA sub-slices (separate PRs): SA-2 move fetch_url.last_fetched_at to a runtime table; SA-3 CI writer-boundary guard; then the reverse-sync teardown.

🤖 Generated with Claude Code

[mousebrains]

Adversarial review of 1defe7a: no findings.

I focused on the operational safety claim: a delete-containing sync without --allow-deletes must return before any DB write, while delete-free updates must still apply on the normal deploy path. The new gate is in the right place: after the read-only plan/dry-run path and before the with conn: write transaction.

Live-DB copy verification using the fresh ../DB/kayak.db:

• Copied the live DB to /private/tmp and built a source.csv diff that both removed source 1 and renamed source 2.
• sync-metadata without --allow-deletes returned 2, printed the observation-drop count for source 1 (10,474 observations), left PRAGMA data_version unchanged on a separate monitor connection, kept source count at 328, kept source 1, and did not apply the rename.
• A delete-free live-copy diff that renamed source 2 returned 0 without --allow-deletes and applied the rename, preserving the normal deploy/CI path.

Additional verification:

• uv run --extra dev pytest -q tests/test_cli/test_sync_metadata.py tests/test_scripts/test_metadata_roundtrip.py tests/test_cli/test_pipeline.py → 38 passed, 1 existing METADATA_DIR deprecation warning.
• git diff --check de31d847c3eef3d2895a9a5917fa25afb92074f3..HEAD clean.
• uv run --extra dev python -m kayak.cli.main sync-metadata --help shows the updated all-or-nothing --allow-deletes wording.
• gh pr checks 146 is green.

Residual notes, not blockers:

• sync-metadata still prints insert/delete counts only; update-only changes can still display as “no changes” in the plan. That predates this PR and is already called out in the runbook.
• With --backup, the backup file is still created before a delete refusal. The reviewed guarantee holds for the live DB state/transaction, not for sidecar backup-file creation.

[mousebrains]

Adversarial review — PR #146 (sync-metadata all-or-nothing delete refusal, SA)

Verified the core fix on a copy of the fresh live DB (629 MB, 432 reaches / 328 sources) — not just the fixture. The change is correct and the bug it fixes is real. One precise, low-severity imprecision in the "no writes on refusal" claim.

The fix works — proven on real data

Constructed a CSV diff with both halves the old code handled inconsistently — an UPDATE (rename source id 1 BCLO3→SYNCTEST_RENAMED) and a DELETE (drop reach 432) — and ran #146's sync-metadata against a copy of the live DB:

• Refused (no --allow-deletes): exit 2, "NO changes applied". DB after: reaches still 432 (delete skipped) and first source still BCLO3 (the UPDATE skipped). This is the bug being fixed — the old code would have committed the rename and only refused the delete, leaving the half-applied state. Now neither lands. data_version and row counts unchanged confirm no transaction committed (AC fetch: don't abort batch on transient errors; close login open-redirect #8).
• --allow-deletes recovery: whole batch applies atomically — "deleted 4 row(s) across 4 table(s)" (reach 432 + its 3 ON DELETE CASCADE children), reaches→431, name→**SYNCTEST_RENAMED**, exit 0. Second run: "no changes — DB already matches", exit 0 (idempotent).

Code-flow confirms it: the refusal moved ahead of with conn:, and nothing between _preflight (read-only) and the gate mutates the DB — the empirical untouched-DB result backs that.

Finding [low]: --backup runs before the refusal gate

sync_metadata runs the --backup block (lines 164–168, writes kayak.db.pre-sync via the online-backup API) before the delete-refusal gate. So on the exact deploy path this PR targets — deploy.sh step 3.1 calls sync-metadata --backup — a refused delete-containing deploy still writes a full ~629 MB backup before returning 2.

• AC fetch: don't abort batch on transient errors; close login open-redirect #8 ("refused deletes begin no write transaction, leave table checksums/counts unchanged") is still satisfied — the backup doesn't touch the live DB's tables (verified: data_version/counts unchanged).
• But the PR's prose ("refused before a single write", "the DB untouched") is imprecise: a backup file is written. It's harmless (same .pre-sync name, overwritten each run, no accumulation) and arguably even desirable, but if "zero writes on refusal" is the intent, moving the --backup block to after the refusal gate makes it literally true — a refused deploy would then do no I/O beyond reading. One-block move; your call whether the pre-refusal backup is a feature (a snapshot right before the operator's manual --allow-deletes) or noise.

Confirmed clean

• deploy.sh step 3.1 change is comment-only — the command (sync-metadata --backup, no --allow-deletes) and the set -e abort-on-exit-2 are unchanged, so deploy behavior is identical except the DB is now untouched on refusal. ✓
• The refusal condition is byte-for-byte the same predicate as before (has_deletes and not allow_deletes), only relocated — so when it refuses is unchanged; only what state it leaves differs. ✓
• The in-transaction if plan.has_deletes: correctly implies --allow-deletes (refusal returned above), so upsert + deletions commit together. The feat(generate-sources): author fetch_url.unknown_station_policy from sources.yaml (S1-fetch-2) #145 _reset_absent_optional_columns (inside upsert_csvs) is also gated behind the refusal now — an all-or-nothing diff holds the policy reset too. ✓
• Tests: the data_version-on-a-second-connection assertion is a sound way to prove no commit; the new whole-batch + pure-upsert cases cover the deploy recovery and common paths.

Net: correct, well-tested, real-data-verified. The only thing I'd want a decision on is the pre-refusal --backup write — keep it (intentional pre-recovery snapshot) or move it after the gate (true zero-write refusal).

🤖 Generated with Claude Code

[mousebrains]

Addressed the --backup-before-the-gate finding in 5e4b541.

Moved the --backup block to after the refusal gate (and after the dry-run return). A refused or dry run now does zero disk I/O — no .pre-sync sidecar is written, so the "refused before a single write / DB untouched" claim is now literally true (file and tables). The backup's purpose is to protect an actual apply (a FK-valid-but-logically-wrong UPDATE that commits and can't be undone from the one-line diff); there's nothing to protect when nothing applies.

Verified on a scratch DB: refused run --backup → exit 2, 0 .pre-sync files; apply run --backup --allow-deletes → exit 0, 1 .pre-sync file (a pre-mutation snapshot: the copy still has the dropped source, the live DB doesn't).

Follow-on: since a refused deploy no longer leaves a snapshot behind, I updated deploy.sh's step-3.1 recovery instruction to levels sync-metadata --allow-deletes --backup — the recovery run is the one that mutates, so it should carry the snapshot. (The common no-delete deploy path still gets its backup: the gate doesn't fire, so the backup runs right before the upsert.)

Tests: split test_backup_writes_pre_sync_snapshot into test_backup_skipped_on_refusal (zero-I/O refusal) and test_backup_writes_pre_sync_snapshot_on_apply (pre-mutation snapshot on apply). Full gate green: ruff/format/mypy, pytest -m "not slow" = 1383, wheel-smoke.

The two other residual notes you raised are intentionally out of scope: the insert/delete-only plan tally (pre-existing, called out in the runbook) and the "would drop 0 observation(s)" wording for non-source-only deletes (pre-existing; the per-table plan above the refusal already shows the non-source deletion). Happy to pick either up as a separate change if you'd like.

Still not merging — yours.