fetch: don't abort batch on transient errors; close login open-redirect - #8
Merged
Merged
Conversation
…atch Phase 3's parse-and-store loop caught (ValueError, KeyError, LookupError) per URL but re-raised everything else, so a single transient failure (network blip, requests.HTTPError, OSError, an internal parser bug, etc.) would tear down the whole run and silently skip every remaining URL that cycle. Treat unhandled Exception the same way as the recoverable parse errors: rollback the per-URL transaction, log with traceback, continue. KeyboardInterrupt and SystemExit are BaseException subclasses, so user-driven aborts still propagate. Test exercises a 2-URL batch where the first parser raises RuntimeError; the second URL must still reach its parser and the session must commit.
The regex \`^/[^/]\` only blocked protocol-relative \`//host\` paths. Per the WHATWG URL spec, browsers normalize \`\\` to \`/\` in special-scheme URLs, so a Location of \`/\\evil.example/\` is rendered as \`//evil.example/\` — a cross-origin redirect after login. Extend the rejection to any second character that is \`/\` or \`\\\`. All other paths (including \`/.well-known/foo\` and querystrings) continue to pass unchanged.
mousebrains
added a commit
that referenced
this pull request
May 10, 2026
Closes the round-2 follow-ups to migration 0013. The most user-visible win is ix_reach_gauge_id — gauge.php's hot query SELECT id, sort_name FROM reach WHERE gauge_id = ? ORDER BY sort_name was planning as SCAN reach USING INDEX ix_reach_sort_name. With this index it becomes SEARCH reach USING INDEX ix_reach_gauge_id (gauge_id=?). The remaining six are FK columns whose ON DELETE SET NULL/CASCADE cascade scans on parent deletion. Closes pre-launch must-fix #8. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mousebrains
added a commit
that referenced
this pull request
May 23, 2026
…able regen Addresses docs/REVIEW_gradient_profile.md findings: * #1 (HIGH): drop hardcoded macOS DB default from the four phase-2 scripts. They now default DEFAULT_DB to "" and exit with "error: pass --db /path/to/kayak.db or set KAYAK_DB in env" if neither is supplied. The macOS path was wrong on both Linux locations; failing loud beats operating on whatever DB happens to match the shape. emit_max_gradient_migration.py docstring updated to match. * #3 (LOW): walk_reach take-out-emit condition rewritten as `cum_m - last_emit_m > 1.0` (the natural reading) with a clearer comment. The old `next_emit_m - interval_m < cum_m - 1.0` form is mathematically equivalent — re-sampling all 407 reaches with the new code produces identical sample counts (359,349), confirming the reviewer's "0.5m short" case is already handled correctly by the 1m close-enough tolerance. Renaming for readability, not changing behavior. * #4 (LOW): drop EPSG:4267 (NAD27) from _GEOGRAPHIC_CRS. NAD27 has a 50-100m horizontal offset from WGS84 across the lower 48 — if a tile shows up tagged 4267, we want the pyproj transform path to correct for that, not silently sample the wrong cell. 3DEP / current OPR tiles are all 4269/4326 so this branch wasn't reached in practice, but the latent risk goes away. * #6 (LOW, a11y): add role="img" + aria-label="Gradient profile chart" to the generated SVG. The hover-replaces-title interaction was leaving keyboard-nav users with no announced label when the chart came into focus mid-interaction. Also locks migration 0046 to byte-stable regeneration: * emit_max_gradient_migration.py: removed the wall-clock `at {datetime.now}` line from the header. Provenance is anchored by the source-DB sha256[:16] which already lives in the comment above. Now a fresh regen against an unchanged DB is byte-identical to the committed file — a `diff` cleanly tells "methodology changed" or "local DB drifted" without false positives from a timestamp. * Migration 0045 docstring updated to match the current JSON shape (top-level `default_rmse_m` / `src_rmse_m` / `src_histogram` after the per-sample-RMSE refactor; `min_drop_ft_for_significance` is now per-window so doesn't live at the top level). * Migration 0046 regenerated against the post-walk_reach-readability cache. Verified byte-identical across two consecutive regens. Findings deferred (not blockers, flagged for follow-ups): * #2: 0046 size — 11 MiB per regen. Reviewer recommends splitting max_gradient (small) from gradient_profile (large) next time methodology changes. * #5: find_tile O(samples × tiles). Fine at ~150 tiles; spatial index when coverage grows. * #8: el._kayakMap cross-script handle. CustomEvent refactor would be more idiomatic. * #9 (NIT): field-order change in 456a37a was in the wrong commit. Retroactive split not worth it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mousebrains
added a commit
that referenced
this pull request
May 23, 2026
Adds a Responses section to docs/REVIEW_gradient_profile.md mapping each finding to the action taken in commit 1ea9a4b plus the verification results. Findings #1, #4, #6 fixed; #3 reworded as readability-only (the "bug" was already correctly handled by the 1m close-enough tolerance, verified by identical sample counts on re-resample); #2/#5/#8/#9 deferred with rationale; #7/#10 confirmed clean. Verification checks #1 and #2 both pass after fixing a stale 0045 docstring and dropping the wall-clock timestamp from the 0046 generator (now byte-stable across regens). Format: one entry per finding, so the next live-host review pass can read top-to-bottom and see exactly what changed vs. what was tabled. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Jun 7, 2026
mousebrains
added a commit
that referenced
this pull request
Jun 8, 2026
…ow-up) Adversarial review of #146 flagged that --backup runs BEFORE the delete-refusal gate, so a refused delete-containing run still wrote the full ~629 MB .pre-sync sidecar before returning 2 — making the "refused before a single write / DB untouched" claim imprecise (AC #8 held for the live DB's tables, but a file was written) and wasting a large write on every refused deploy. Move the --backup block to AFTER the refusal gate (and after the dry-run return), so a refused or dry run does ZERO disk I/O. 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 is nothing to protect when nothing applies. The online-backup still runs on its own fresh connections, independent of the sync connection. Because a refused deploy no longer leaves a snapshot behind, update deploy.sh's step-3.1 recovery instruction to `sync-metadata --allow-deletes --backup` — the recovery run is the one that mutates, so it should carry the snapshot. Tests: split test_backup_writes_pre_sync_snapshot into test_backup_skipped_on_refusal (refused run with --backup writes no .pre-sync — zero-I/O refusal) and test_backup_writes_pre_sync_snapshot_on_apply (an apply with --backup writes a pre-mutation snapshot: the copy still has the dropped source, the live DB does not). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mousebrains
added a commit
that referenced
this pull request
Jun 8, 2026
…/update apply (SA) (#146) * fix(sync-metadata): refuse deletes all-or-nothing — no partial insert/update apply (SA) Dataset-separation SA acceptance criterion #8: "Refused metadata deletes begin no write transaction and leave logical table checksums/counts unchanged." Today `levels sync-metadata` does the opposite — when a CSV diff contains deletions and `--allow-deletes` is not passed, it still runs `upsert_csvs()` (committing every insert/update) and only refuses the deletes (exit 2), leaving a half-applied DB. Move the refusal BEFORE the write transaction: if the plan has deletes and `--allow-deletes` is absent, print the plan + per-source observation-drop counts (already computed by `compute_plan` and printed by `_print_plan`, both pre-transaction) and return 2 with zero writes. The in-transaction delete branch is now reached only when `--allow-deletes` is set, so a recovery run applies the whole batch (inserts/updates AND deletes) atomically — and is idempotent. The deploy flow is unchanged and 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 leaves the DB untouched instead of 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. Add `test_refused_delete_then_allow_deletes_applies_whole_batch` (the deploy recovery flow: refuse → byte-unchanged → `--allow-deletes` applies the whole batch → idempotent) and `test_pure_upsert_without_allow_deletes_applies` (a delete-free diff still applies without the flag — guards the common deploy + kayak_data CI path). Sweep the module docstring, `--allow-deletes` help, `deploy.sh` step-3.1 comment, and the add-gauges runbook to describe all-or-nothing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(sync-metadata): take --backup after the refusal gate (review follow-up) Adversarial review of #146 flagged that --backup runs BEFORE the delete-refusal gate, so a refused delete-containing run still wrote the full ~629 MB .pre-sync sidecar before returning 2 — making the "refused before a single write / DB untouched" claim imprecise (AC #8 held for the live DB's tables, but a file was written) and wasting a large write on every refused deploy. Move the --backup block to AFTER the refusal gate (and after the dry-run return), so a refused or dry run does ZERO disk I/O. 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 is nothing to protect when nothing applies. The online-backup still runs on its own fresh connections, independent of the sync connection. Because a refused deploy no longer leaves a snapshot behind, update deploy.sh's step-3.1 recovery instruction to `sync-metadata --allow-deletes --backup` — the recovery run is the one that mutates, so it should carry the snapshot. Tests: split test_backup_writes_pre_sync_snapshot into test_backup_skipped_on_refusal (refused run with --backup writes no .pre-sync — zero-I/O refusal) and test_backup_writes_pre_sync_snapshot_on_apply (an apply with --backup writes a pre-mutation snapshot: the copy still has the dropped source, the live DB does not). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two unrelated but small fixes surfaced by a deep-dive of fetch.py and the PHP auth helpers.
Summary
Test plan
🤖 Generated with Claude Code