Reduce DB bloat, improve lock contention, add gauge metadata cache - #2
Merged
Conversation
…aceholder pages - index.html is now the Oregon levels table (replaces landing page card grid) - Nav bar uses state abbreviations (Map | AZ | CA | ... | OR | ... | WY | Picker) - OR links to /index.html, other states link to placeholder pages - GeoJSON and map filtered to Oregon reaches, centered on Oregon - all.html no longer generated - Pipeline still fetches/processes all states; reverting = checkout main + rebuild Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… directory Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Each state page has links to AW, Dreamflows, USGS, RFC, USBR Hydromet, local paddling clubs, and Windy weather. Oregon.html also generated. Nav bar includes OR Weather link to Windy. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- New nwrfc_textplot parser for NWRFC textPlot.cgi data source - Dreamflows import script for caching reach/flow data - Reach search: add state filter, Leaflet map, flow/gage readings, class and guidebook columns - Merge: use ±15min moving median window across all sources (including single-source gauges), delete stale target rows before writing - Description page: reduce coord precision to 5 decimals, fix NWS/NWRFC agency matching for station links - Update reaches.geojson Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove dreamflows_cache.json from .gitignore and add data/dreamflows_cache.json to the repo so it's available without regenerating. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ments Add dual-axis SVG plot (flow + gage height), increase plot font sizes. Show reach description in search results and build output. Handle NWPS -9999 sentinel, allow negative USGS gage heights. Normalize observation timestamps to second precision. Update systemd services from tpw to pat paths. Use deterministic timestamps in parser tests. Add tkw.*/tpw.* to .gitignore. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The merge step was processing all 1,938 gauges (including 1,692 single-source gauges) × 4 data types, needlessly reading, deleting, and re-inserting 2.5M observations. Skipping gauges with <2 sources cuts merge time from 186s to 47s, keeping the pipeline well within its 5-min systemd timeout. Also: - Flush pipeline print() calls so step timings appear in journalctl - Use absolute paths in kayak-sync.service (fixes %h expansion issues) - Increase sync timeout to 1800s, add --days 7 to observation sync Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fetch inflow observations for sparklines (gauges with DataType.inflow) - Add Subresource Integrity hashes to Leaflet JS/CSS on map page - Add privacy policy link to page footers - Rebuild reaches.geojson with updated geometries Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…cy policy - edit.php: require EDIT_PASSWORD (403 if unset), add CSRF token, harden session cookie (Strict SameSite, HttpOnly, Secure, scoped to /edit.php) - api.php: cap days parameter at 365 to prevent resource exhaustion - privacy.php: new privacy policy page with accurate cookie disclosure - nginx config: move password to restricted snippet, add PHP rate limiting, fix /includes/ blocking with regex location, hide server version - security-harden.sh: sudo script for fail2ban jail, php.ini hardening, password rotation, service reloads Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extracts LineString geometry from AW's Mapbox Vector Tiles when the GraphQL API is unavailable. Chains per-tile segments and optionally saves to the database. Requires mapbox-vector-tile pip package. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- php/style.css: copy of stylesheet for PHP pages to use via header.php - header.php: resolve CSS from php/style.css instead of src/ path - robots.txt: disallow /edit.php from crawlers Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add object-src 'none' and base-uri 'self' to CSP per pentest findings - Use more_clear_headers to fully remove Server header - Add libnginx-mod-http-headers-more-filter install to hardening script Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
mkstemp() creates files with mode 0o600, which after rename left output files unreadable by www-data. chmod before replace. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
script-src 'self' was blocking the three inline <script> tags (local time conversion, clickable rows, service worker registration). Consolidated into src/kayak/web/static/levels.js, copied to output static/ during build. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extracted inline Leaflet map initialization from description.php and
reach.php into static/reach-map.js and static/search-map.js, passing
data via data-* attributes. Unified nginx CSP by replacing inline
headers in deploy/levels with include of conf/security-headers.conf
across all location blocks. Added bare tile.openstreetmap.org domain
to img-src and standardized tile URLs to use {s} subdomains.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…o custom page - Extract picker.php inline script to static/picker.js - Replace footer.php inline scripts with levels.js include - Add SVG sparklines and clickable rows to custom.php - Use <time> elements in custom.php for client-side timezone conversion - Scope picker to Oregon-only reaches, auto-load on page open Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sparkline query fetched 177K rows via fetchAll() which exceeded the 128MB PHP-FPM memory limit. Changed to per-gauge queries with downsampling to ~60 points each, reducing peak memory from 128MB+ to 6MB. Also use <time> elements for client-side timezone conversion. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reformatted flow levels from multi-row to 2-row table with Low, Okay, High as centered columns. Updated reaches.geojson from latest build. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Search results map (reach.php) now draws actual river traces from geom data instead of straight put-in/take-out lines, downsampled to ~100 pts - Search map markers placed at trace midpoint when available - Reach detail map (reach-map.js) draws dashed lines connecting put-in/ take-out markers to trace endpoints - Cap custom page reaches at 200 (user change) - Updated reaches.geojson Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Generated build artifact that changes every pipeline run. Rebuilt from database by `levels build`. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Commit after each gauge/source in calc_rating, calculator, merge to release the SQLite write lock sooner - Rollback on per-item errors instead of accumulating in a single txn - Increase busy_timeout from 5s to 30s for concurrent access - Scope decimate SQL to one source_id at a time so deletes are batched - Close session after phase 1 in fetch before starting HTTP I/O - Refactor fetch_usgs_ogc to separate DB reads, HTTP, and DB writes - Filter stale NWPS observations older than 36 hours - Update tests for refactored code Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Move AW reaches and Dreamflows data from JSON files into Gauge-metadata-cache/gauges.db (SQLite) with USGS and NWPS site metadata tables - Update match_aw_reaches, import_dreamflows, and import_aw_usgs_reaches scripts to read/write SQLite instead of JSON - Add fetch_usgs_sites.py and fetch_nwps_sites.py for populating the gauge metadata cache - Comment out bulk USGS state fetches in sources.yaml — the OGC fetcher covers all gauges linked to reaches without creating unlinked sources - Remove unused sources.old.yaml - Add OSM-cache/ to .gitignore Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
mousebrains
added a commit
that referenced
this pull request
May 10, 2026
The single auth zone (3 r/m) shared by /login.php, /auth.php,
/logout.php, and /contact.php meant any one of them could lock the
others out. Most damaging path: anonymous contact-form spam (50/day at
1% of 5k DAU) saturating the bucket and blocking magic-link logins.
New layout:
- login (3/min, burst=2) — magic-link issuance; each request emails
- auth (10/min, burst=4) — token consumption + logout; brute-safe
(32-byte token), so rate is relaxed
- contact (10/min, burst=4) — anonymous, gated by Turnstile + honeypot
Verified post-reload: login lockout does not leak into contact or auth;
each zone has the expected fresh-bucket size of 1+burst.
Closes pre-launch must-fix #2.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mousebrains
added a commit
that referenced
this pull request
May 12, 2026
Three findings from the Tier 2 + Tier 3 review of `e778053..a6d5242`; all test-file edits, no production code touched. Finding #1 — Lost CSP regression guard. Phase 1.4's lone drill test (`d3e7dce` tests/php/ReachIntegrationTest.php) had: $this->assertStringNotContainsString( '<script>', $resp['body'], 'inline <script> would clash with prod CSP' ); That assertion was dropped when ReachIntegrationTest expanded from 1 to 6 tests in Phase 2.1 and never made it into DescriptionIntegration Test in Phase 3.1. The CSP-header check (`assertArrayNotHasKey(' content-security-policy', $resp['headers'])`) was preserved on the detail tests, but the body-side guard against re-introducing bare inline `<script>` was lost. Production runs strict CSP — bare inline scripts would break, but the test suite would stay green. Restore as `assertNoBareInlineScript($body)` in IntegrationTestCase and call from every HTML-rendering test in both Reach (5 calls) and Description (4 calls — 400 cases don't render HTML). Catches bare `<script>inline</script>` but allows `<script src=...>` (the attribute value pushes the closing `>` past the substring match — works for both Leaflet and the page-specific maps). Finding #2 — `ReachIntegrationTest.php:119` comment was wrong about which columns the LIKE pattern matches. The comment claimed "'A test reach' description for both — the OR-of-LIKEs picks both", but _search_reaches_query LIKEs display_name OR name OR river — NOT description. The test still passes because both reaches have 'Test' in their name/display_name; only the comment misleads. Updated to spell out the actual matching columns per row. Finding #4 — `DescriptionIntegrationTest.php:143` comment said 'Sandy' pins "// River field", but description.php's $fields list doesn't include a 'River' label. 'Sandy' actually matches via the display_name "Sandy Test Reach" in <title>, <h2>, and the meta description. The same test class already had a longer comment on the no-gauge case (line 159-163) explaining this exact mechanism for "Nowhere" — the gauged-reach test was inconsistent with that explanation. Reworded the comment, kept the assertion (still valid). Finding #3 (dead `$html_fields` entry 'AW ID' at php/includes/reach_detail.php:350) is a pre-existing carryover from the inline reach.php code, not introduced by Tiers 2/3. Flagged for a separate cleanup pass. Test/lint state: - All 12 integration tests pass with the new guard active (full PHPUnit: 67/67, +9 assertions) - PHPStan + php-cs-fixer green Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mousebrains
added a commit
that referenced
this pull request
May 12, 2026
Tier 4 review finding #1 — testRawSvgResponse used assertSame on the content-type header, which would break if PHP-FPM ever appends "; charset=utf-8" via a default_charset config tweak. That's not a real regression — only the "image/svg+xml" media-type prefix matters for browser / CDN handling — so loose-match it with assertStringStartsWith to mirror the HTML test's check at the embed assertion. Findings #2 (Cache-Control: max-age=300 un-asserted) and #3 (derive_rating_lookup self-join is potentially expensive on heavily-populated gauges) noted in the review as informational follow-ups: the cache-control assertion can be added if/when the policy ever shifts; the self-join is pre-existing pre-Tier-4 code worth a covering index only if it shows up in slow-query logs. Full PHPUnit 72/72 still green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mousebrains
added a commit
that referenced
this pull request
May 12, 2026
… review) Two test-only edits per the Tier 5.G review of `8a3fa62 + d46f5e2 → 1042cc3`: Finding #1 — Stale line-range reference. The comment in testSearchModeMultiMatchRendersTable said "Search SQL (gauge.php:27-34) selects/renders the `name` column" — accurate when written in Phase 5.G.1 but stale after Phase 5.G.2 (`5ac3a57`) moved the SQL into gauge_search.php:_search_gauges. Update to a file:function reference which survives future line-motion better than file:line. Finding #2 — Single-match query fragility. testSearchModeSingleMatchRedirects relied on 'CLACKAMAS_TEST' being a unique substring of the seeded canonical names — true today but could silently regress if a future seed adds another row with a colliding substring. (Wouldn't be silent — `assertSame(302, …)` would fail loudly with "expected 302 got 200" — but the failure message wouldn't point at the root cause.) Rename canonical seed names to embed the gauge ID: 'CLACKAMAS_TEST' → 'CLACKAMAS_TEST_UNIQ_4001' 'EMPTY_GAUGE_TEST' → 'EMPTY_GAUGE_TEST_UNIQ_4002' Single-match query changes from 'CLACKAMAS_TEST' to 'UNIQ_4001' — unique-by-construction (the suffix can't accidentally appear in any other test's seed). Multi-match assertions updated to check for the new canonical names. The display_name constants stay unchanged so the detail-mode test assertions are unaffected. The seed comment now calls out the uniqueness convention so future test writers see the pattern. Test/lint state: - All 7 GaugeIntegrationTest cases pass (full PHPUnit: 79/79) 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 May 31, 2026
mousebrains
added a commit
that referenced
this pull request
Jun 4, 2026
install-config.sh (MEDIUM): the secrets sanity check enforced only a non-empty TURNSTILE_SECRET via a bare grep, so a fresh host could pass the installer with the site key missing — and turnstile_enabled() requires BOTH JSON keys, recreating the captcha-silently-off failure this PR closes. The guard now requires both keys and parses with the SAME semantics as the install wrapper's merge (export prefix, quote strip, empty = disabled — the old grep also rejected export-style lines and accepted quoted-empty values). A no-root `--check-secrets [FILE]` mode makes the real script testable; 7 new tests include a guard↔wrapper parity case pinning that any file the guard accepts merges both keys. review_send_reply (minor #1): the reviewer-note merge happened in PHP from the request-start row, so two concurrent reply tabs (both passing the `pending` predicate — replies don't flip status) would last-writer-win and drop the first reply's note. The append now happens SQL-side inside the atomic UPDATE; reviewer_note_entry() extracted so the stamp format stays shared with merge_reviewer_note(), whose docstring now explains why terminal actions can keep the PHP-side merge. Regression test drives the stale-row shape and asserts both notes survive. validate-config (minor #2): add the SQLITE_ prefix and allowlist SQLITE_PATH (PHP db.php fallback + health-check.sh DB override; not a model field — python uses DATABASE_URL). Strict scan re-verified rc=0 against the live host's env set with the new prefix active. health-check.sh (minor #3): comment the bare fu.is_active under GROUP BY s.id — SQLite's bare-column extension, safe because is_active is functionally dependent on s.id; flags it against a future "fix" to MAX() or an unexamined port to a stricter engine. Findings #4 (wrapper merges all secrets.env keys — observation, no action; documented in secrets.env.example) and #5 (deploy-day transient — acknowledged in the scope notes) per the review need no change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mousebrains
added a commit
that referenced
this pull request
Jun 4, 2026
…crets merge, runbook fixes (#119) * fix: address 2026-06-03 gpt-5.5 project-review findings health-check.sh (HIGH): the script only checked the single global MAX(observed_at), so a dead feed — or a source that never produced data at all — was invisible while any other source kept writing, and the healthchecks.io heartbeat stayed green. Add a per-source liveness check: active gauge-linked fetch-backed sources fail when never-fed or silent > STALE_SOURCE_DAYS (default 14, env-overridable, numeric-guarded against silently disabling the SQL); OGC-fetched USGS sources (selected by agency + gauge link — `fu.is_active IS NOT 1` so an inactive-fetch_url USGS source isn't a scope hole) get the same silent->fail window once fed, but never-fed ones are exempt as speculative metadata additions awaiting upstream OGC coverage (operator decision 2026-06-03). Reconcile docs/slo.md SLO F to what is actually enforced — the documented per-source "cadence + 2 h" model never existed anywhere. 12 regression tests run the real script against an ORM-built SQLite DB (tests/test_scripts/test_health_check.py). Verified against the prod snapshot: 0 offenders at the 14-day window, so the new check is green on deploy day. config-refresh runbook (MED): docs/operations.md still told the operator to run the old `sudo levels emit-config --out ...` — the exact pat->root RCE the review-3 R1.5 wrapper eliminated. Rewrite to the real flow (`emit-config --dry-run` piped into the root-owned kayak-install-runtime-config) and document the one-time wrapper + sudoers install. Also fix deploy/SETUP.md's claim that the §7 sudoers grant invokes the pat-writable venv levels binary. local PHP quick start (MED): README, CLAUDE.md, and .env.example all ended with a bare `php -S`, which 500s every dynamic page since Config became fatal-on-missing (T3.3 Phase 4). Add the `levels emit-config --out` + KAYAK_CONFIG_PATH step everywhere and correct the stale "PHP gets SQLITE_PATH from nginx fastcgi_param" claims (database_path from the runtime-config JSON first, SQLITE_PATH env fallback). Smoke-tested locally: 200 with the config step, 500 without. review reply race (LOW): review_send_reply() updated reviewer_note with no status predicate, so a stale "reply, keep pending" tab could mutate an already-approved/rejected row and email the editor a misleading "still pending" message. Use the same atomic `WHERE id = ? AND status = 'pending'` + rowCount() pattern as the terminal actions, return bool, and surface "Already reviewed by another maintainer." in the handler. Race regression test added. validate-config (LOW): add METADATA_ and USGS_ to the scanned prefixes, declare hc_fetch_osmb/hc_status (referenced by their systemd units but never added as fields — either would have failed the first strict deploy), and allowlist KAYAK_DATA / KAYAK_VENV / USGS_API_KEY (env-read names that must not become model fields; USGS_API_KEY in particular would otherwise be emitted into the www-data-readable runtime-config.json). Flip scripts/deploy.sh to `validate-config --known-env --strict`; verified rc=0 against the live host's actual .env contents. stale docs (LOW): drop the long-dead `pages` table from db_sync.md and export_metadata.py's docstring, and correct the levels-test.wkcc.org/_internal/ claims — that host 301s wholesale to levels.wkcc.org since 2026-05-19. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: merge secrets.env into runtime-config JSON in the install wrapper gpt-5.5 take-2 review (2026-06-03), MEDIUM, confirmed FIRED in production: the live login page serves no Turnstile widget — captcha is silently off. Root cause is the collision of two individually-sound changes: review-3 R1.5 moved the runtime-config render to unprivileged pat (`levels emit-config --dry-run`), which cannot read /etc/kayak/secrets.env (0600 root:www-data), so TURNSTILE_SITE_KEY / TURNSTILE_SECRET vanish from the JSON; and T3.3 Phase 4 removed PHP's getenv fallback, so the FPM-pool env re-export no longer rescues it. turnstile_enabled() then false-paths to turnstile_verify() === true. config.py's secrets.env comment still described the pre-R1.5 root-rendered flow that made this work. Fix: the root-owned kayak-install-runtime-config wrapper now merges secrets.env into the piped JSON before installing — each KEY=VALUE lands as lowercase(KEY) unless the rendered JSON already carries a non-empty value (parity with config.py's load_dotenv override=False precedence). This keeps both prior security decisions intact: the R1.5 boundary (no pat-writable code runs as root; the merge code is inside the fixed root-owned wrapper) and Phase 4's JSON single-source-of-truth. `export KEY=VALUE` lines are accepted, matching python-dotenv and systemd EnvironmentFile parsing of the same file (adversarial-review finding: without that strip an export-style secrets.env would mint a bogus "export turnstile_secret" key and re-create the captcha-off bug). Test hooks KAYAK_INSTALL_DEST / KAYAK_INSTALL_SECRETS are honored only when euid != 0 — as root (the sudoers entry) the paths stay fixed, and sudo's env_reset (no SETENV tag) strips the vars anyway, so the hooks add no escalation surface. 10 new wrapper tests run the real script (tests/test_scripts/test_install_runtime_config.py); a new PHP test drives the full production shape end-to-end (pat-shaped JSON without turnstile keys + secrets.env -> wrapper -> Config -> turnstile_enabled() true). Prose swept to match: config.py secrets comment, turnstile.php docblock (claimed a getenv fallback Phase 4 removed), kayak-fpm-pool.conf (its env re-export is now an unread legacy channel), secrets.env.example (documents the merge flow + both keys), SETUP.md, operations.md, deploy.sh step 3.5. Also annotated the Phase 4.3 draft SLO targets in PLAN_production_discipline.md as superseded by docs/slo.md (take-2 residual note). Deploy note: the wrapper at /usr/local/sbin is a copied artifact — the fix is live only after `sudo install -m 0755 -o root -g root deploy/kayak-install-runtime-config.sh /usr/local/sbin/kayak-install-runtime-config` and a deploy (or the emit|install step) re-renders the JSON. Until then kayak-config-drift.service flags the stale copy (byte-exact cmp). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: archive the gpt-5.5 project-review reports (2026-06-03) Both rounds of the external review whose findings the two preceding commits address, archived per the docs/done/REVIEW_* convention. Round 1: healthcheck per-source freshness (HIGH) + runbook/quick-start staleness + review-reply race + validate-config gaps. Round 2 (take 2, reviewing commit 39e7b6c): the Turnstile secrets-merge hole in the runtime-config install pipeline, confirmed fired in production. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: index the archived gpt-5.5 reviews in docs/done/README.md test_docs_done_plans_are_indexed (review-4 R2.3 guard) requires every docs/done/REVIEW_*.md to have an index row — fc8c16d archived the two reports without one, which is exactly the drift the guard exists to catch. CI red on #119; lesson re-learned: docs-only commits still get the full test gate in this repo. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: address PR #119 review findings install-config.sh (MEDIUM): the secrets sanity check enforced only a non-empty TURNSTILE_SECRET via a bare grep, so a fresh host could pass the installer with the site key missing — and turnstile_enabled() requires BOTH JSON keys, recreating the captcha-silently-off failure this PR closes. The guard now requires both keys and parses with the SAME semantics as the install wrapper's merge (export prefix, quote strip, empty = disabled — the old grep also rejected export-style lines and accepted quoted-empty values). A no-root `--check-secrets [FILE]` mode makes the real script testable; 7 new tests include a guard↔wrapper parity case pinning that any file the guard accepts merges both keys. review_send_reply (minor #1): the reviewer-note merge happened in PHP from the request-start row, so two concurrent reply tabs (both passing the `pending` predicate — replies don't flip status) would last-writer-win and drop the first reply's note. The append now happens SQL-side inside the atomic UPDATE; reviewer_note_entry() extracted so the stamp format stays shared with merge_reviewer_note(), whose docstring now explains why terminal actions can keep the PHP-side merge. Regression test drives the stale-row shape and asserts both notes survive. validate-config (minor #2): add the SQLITE_ prefix and allowlist SQLITE_PATH (PHP db.php fallback + health-check.sh DB override; not a model field — python uses DATABASE_URL). Strict scan re-verified rc=0 against the live host's env set with the new prefix active. health-check.sh (minor #3): comment the bare fu.is_active under GROUP BY s.id — SQLite's bare-column extension, safe because is_active is functionally dependent on s.id; flags it against a future "fix" to MAX() or an unexamined port to a stricter engine. Findings #4 (wrapper merges all secrets.env keys — observation, no action; documented in secrets.env.example) and #5 (deploy-day transient — acknowledged in the scope notes) per the review need no change. 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 was referenced Jun 4, 2026
mousebrains
added a commit
that referenced
this pull request
Jun 14, 2026
…lure (errtrace) Adversarial review of #192 surfaced a chain of rollback gaps. Fixing finding #1 (arm DOCROOT_BUILT before the build) exposed a deeper, pre-existing PR #190 bug: the rollback never ran for the activation mutation steps at all. - **errtrace (the big one).** The deployer ran `set -euo pipefail` without `-E`. Every DB/build mutation runs through `run_app()` (a function), and without errtrace a failure INSIDE a function aborts under `set -e` WITHOUT triggering the `ERR` trap. So a failed `run_app … migrate/sync/import/build` exited with the DB already mutated, consumers stopped, maintenance still on — and NO rollback (only the EXIT scratch-cleanup). `set -Eeuo pipefail` makes those failures hit `trap rollback ERR`. The trap is armed for the activation phase only (staging has no ERR trap, so `-E` is inert there); atomic_relink's `mv` probes sit in `if` conditions, which stay exempt. Surfaced writing finding #1's test — the docroot-rebuild fix is moot if rollback doesn't run. - **#1: DOCROOT_BUILT armed before the build, not after.** `build` writes the live docroot in place (stage → per-file rename → orphan sweep), so a failure mid-write leaves it mixed yet exits non-zero. The flag must mean "build started" (docroot may have changed) so rollback rebuilds; a rebuild when it was in fact untouched is harmless/idempotent. - **#3: normalize an absolute `current` symlink target.** The rebuild used `$ROOT/$PREV_TARGET`; a manual-recovery absolute `current` made that `/opt/kayak//opt/...`. Compute `PREV_DIR` with an absolute/relative case split. - **nit #4: keep the rollback rebuild's stderr** (to `$SCRATCH/docroot-rebuild.err`, retained) instead of discarding it — the recovery path is where the reason matters most. - **#2 (ACL): commit the shared-docroot web ACL** to deploy/SETUP.md's cutover section — the recursive + default `u:www-data:rX` on `/var/cache/kayak/docroot` the deployer doesn't grant; without the `-d` default ACL each freshly built page 403s. The nginx-root / FPM-open_basedir / unit verification gate stays a 4C-renderer deliverable (it owns those templates). Tests: two new slow tests — build mutates the docroot then exits 1 → rollback rebuilds (proves the flag-before-build + errtrace fix); and an absolute `current` target → rebuild reads the normalized dir. Full slow suite 5 passed, non-slow 1725 passed, ruff/format/shellcheck clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mousebrains
added a commit
that referenced
this pull request
Jun 14, 2026
…+ harden paths 1. (Medium) The serving renderer was an un-pinned second source of truth for open_basedir/root: the tests asserted hardcoded literals, so a path added to the committed pool would leave the renderer silently incomplete, CI green, and PHP losing that path at cutover (open_basedir REPLACES, not extends). Added tests that parse `deploy/kayak-fpm-pool.conf` and `conf/snippets/levels-common.conf` and assert the default-HostConfig render reproduces them byte-for-byte — turning the parity claim into an enforced invariant. The nginx test also asserts there's exactly one non-certbot `root`, so the two-root structure (#2) is guarded. 2. (Low, forward-looking) Noted in the plan that the cutover apply step (increment 4) must target the docroot `root` (~line 30), NOT the ACME `root /var/www/certbot;` (~line 305) — a blanket root-sed would break renewal — and the serving-path gate should verify the certbot root survives. The new test enforces the one-docroot-root structure so a regression trips CI. 3. (Low) `_PATH_BAD_CHAR_RE` now also rejects the config delimiters `:;{}#` — the path fields land in colon-delimited open_basedir and `;`-terminated nginx directives, so a `:` would SILENTLY widen PHP's sandbox and a `;` end a directive early. host.yaml is trusted, so defense-in-depth. 4. (Nit) Strengthened the RUNTIME_CONFIG_PATH comment: it MUST stay in lockstep with the deployer's KAYAK_RUNTIME_CONFIG or open_basedir points at the wrong config and 500s PHP. Full non-slow suite 1761 passed; ruff/format/mypy clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jun 14, 2026
mousebrains
added a commit
that referenced
this pull request
Jun 15, 2026
…add tests - #1 (the conscious trade-off): document the KNOWN LIMITATION inline — masking the whole line means config-drift no longer verifies the *content* of `root` / `open_basedir` between deploys (a manual tamper of just those lines wouldn't be flagged). Note that the deployer's serving-path gate still verifies them at deploy time, and record the verify-against-`render-serving` upgrade as the follow-up that closes the continuous-monitoring gap. - #2: comment the FPM-pool-path duplication (RENDER_NORMALIZED + MANIFEST) so a PHP-version bump updates both, not just one. - #4: add tests/test_scripts/test_config_drift.py — sources the script in a new lib mode (KAYAK_DRIFT_LIB=1, early-return before the /etc manifest walk) and drives normalize_rendered directly, locking the surgical mask: the docroot `root`/`open_basedir` are exempt, but the ACME `root /var/www/certbot;` stays byte-checked (a tampered ACME root still differs). Guards the certbot exclusion against a future sed edit. #3 (the favicon/security.txt aliases re-introducing drift) is mooted by PR #199, which makes those aliases docroot-relative (try_files) rather than hardcoding the cache path — so they stay generic and need no mask. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mousebrains
added a commit
that referenced
this pull request
Jun 15, 2026
…view #2) A one-line regression lock requested in review: assert the committed levels-common.conf has no `alias ...public_html...` line. Pins the cutover follow-up #2 invariant — the favicon/security.txt locations serve docroot-relative via `try_files /static/... =404;`, so a future edit can't silently reintroduce an absolute public_html alias (which would 404 after public_html cleanup and diverge from the live file, since config-drift doesn't mask alias lines). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mousebrains
added a commit
that referenced
this pull request
Jun 15, 2026
fix(nginx): serve favicon/security.txt docroot-relative (cutover follow-up #2)
This was referenced Jun 18, 2026
Merged
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.
Summary
Gauge-metadata-cache/gauges.db(SQLite), added USGS and NWPS site metadata tables for gauge discovery.Test plan
pytestpasseslevels pipelineruns without lock timeout errorslevels fetch-usgs-ogc🤖 Generated with Claude Code