Rewrite C++ CGI to Python package with PHP web layer - #1
Merged
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Translates the entire C++ water-level data aggregation system to Python using Flask, SQLAlchemy, Matplotlib, and Click. Consolidates 3 MySQL databases into a single schema with a unified measurements table. - Foundation: pyproject.toml, SQLAlchemy models (14 ORM classes), config, engine factory, CLI entry point, database init with seed data - 19 data parsers (USGS, NOAA, NWRFC, USBR, USACE, CBRFC, IDWR, etc.) with decorator-based registry replacing makeParser() if/else chain - Pipeline commands: fetch, merge, calc-rating, calculator, build, pipeline - Flask web app with 6 route blueprints, legacy CGI URL compatibility, Matplotlib time-series plotting, JSON data API - Alembic migration setup for production MySQL - 22 passing tests against in-memory SQLite Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the flat Master/MergedMaster/Correction schema with the production 16-table normalized schema (Gauge, Source, GaugeSource, FetchUrl, Section, Observation, LatestObservation, Rating, etc.). Move URL/parser definitions, builder columns, and description fields from DB tables to YAML config files (data/sources.yaml, data/builder.yaml, data/descriptions.yaml). Update all parsers, CLI commands, web routes, and tests for source_id-based queries and lowercase DataType enum values (flow/gauge/inflow/temperature). All 33 tests pass. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Rename the CLI entry point from 'kayak' to 'levels' to match the project's domain. Add scripts/import_from_dump.py which parses a mysqldump of levels_todo and populates the local SQLite database with the normalized schema (states, gauges, sources, sections, ~12M observations, latest_observation cache). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Allows testing parsers against previously saved pages from --output-dir without hitting the network. Extracts content retrieval into _get_content() helper shared by both the main loop and _fetch_single. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Group URLs under their parser name as the top-level key, dropping the redundant per-entry parser and group fields. Rename usgs.rdb to usgs to match the registry. Mark cdec, cdec.station, and crohms as enabled: false since they have no Python parser implementations yet. Update load_sources() to flatten the new structure back to the same list-of-dicts interface so no downstream callers change. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Delete parser implementations that have no associated data source URLs: cbrfc, idahoPower, idwr, noaa, noaa.xml, noaa2, nwrfc, ocs, ocean.newport, usace.ca, usace.resv, usbr.pn, usbr.special. Update registry.py to only import the 5 active parsers: usgs, nwrfc.xml, usbr, usace.outflow, wa.gov. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Create src/kayak/cli/logger.py with add_logging_options() decorator and setup_logging() mirroring TPWUtils/Logger.py: RotatingFileHandler, optional SMTPHandler, mutually exclusive --debug/--verbose flags. Wire logging into the CLI group in main.py so all subcommands inherit it. Remove duplicated --verbose options and logging.basicConfig() calls from fetch, build, calc_rating, calculator, merge, and pipeline. Convert verbose-gated click.echo() calls to logger.debug()/logger.info(). Remove verbose parameter from BaseParser. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The entire C++ codebase (168 .C/.H files), Makefiles, scripts/master, files/, and gen.sql/ have been superseded by the Python translation. Retain scripts/import_from_dump.py which is still used. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace Click decorators with argparse add_subparsers pattern matching the TPWUtils addArgs/mkLogger convention. Each subcommand module now exposes addArgs(subparsers) and a run function. Pipeline calls subcommand functions directly instead of ctx.invoke(). Remove click from dependencies. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The calc expressions from the production dump use a 3-part format (key::source_name::type) but the parser assumed 2-part. Also add gauge name resolution so merge target names like Mohawk_Springfield_merge resolve through the Gauge→GaugeSource→Source chain. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract fetch options into addArgs_options() so both the fetch subcommand and pipeline subcommand can reuse them. This allows running the full pipeline with fetch-specific flags like `levels pipeline -i -I /tmp/fetched-pages`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…to 76% - Remove C++ artifacts (.orig files, mkFlows binary, tpw graphics) and empty dirs - Extract get_primary_source_id/get_source_ids_for_gauge helpers into info_db.py and update 7 callers across CLI and web routes - Fix == False to .is_(False), remove unused imports, move lazy import re to module level, fix deprecated datetime.utcnow() calls - Normalize error handling: replace print(stderr) with logger.error() in all CLI - Configure ruff (E/W/F/I/UP/B/SIM/RUF) and mypy in pyproject.toml - Apply ruff auto-fixes: StrEnum migration, UP037/UP045 cleanups, zip(strict=True) - Fix all mypy errors (type annotations, Any returns, import-untyped) - Add 200 new tests (233 total): utils, parsers, DB, CLI, web routes, plotting - Add pytest markers, coverage config, and dev deps (ruff, mypy) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…elivery The Flask/matplotlib stack required Passenger/WSGI on DreamHost and added unnecessary round trips. This replaces it with static HTML files built to disk by `levels build` plus lightweight PHP scripts for dynamic routes, all served directly by Apache. Key changes: - build.py writes self-contained HTML to public_html/ with inlined CSS, responsive mobile-first card layout, inline SVG sparklines, and direct links (no more ?D=/?f= redirects) - PHP scripts handle dynamic content (description, plot, api, latest, view, edit) with shared includes for DB, header/footer, LTTB, SVG plots - Lightweight SVG plot generation replaces matplotlib (3-8KB vs 20-100KB) - LTTB downsampling in both Python (sparklines) and PHP (plots, API) - PWA support: service worker (network-first/cache-fallback), manifest, single 192px icon replaces 467KB of apple-touch-icon variants - Apache .htaccess with mod_deflate, cache headers, error pages - Remove flask, Jinja2, matplotlib, httpx dependencies - Remove web.templates/ (30 legacy CGI/template files) - All 208 tests pass, zero ruff violations Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
New picker.php lets users select states, search/filter sections, and build a custom levels page from checked selections. Custom page URL (?ids=1,2,3) is bookmarkable and shareable with no server-side persistence needed. - php/picker.php: state pill filters, AJAX section endpoint, text search, section checklist with sticky action bar - php/custom.php: renders standard levels table for arbitrary section IDs, same HTML structure/classes as static state pages - header.php: add Picker nav link with active state support - style.css: picker-specific styles (pills, search, table, action bar) - public_html symlinks for both new pages Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
MySQL uses GROUP_CONCAT(name SEPARATOR ', ') while SQLite uses GROUP_CONCAT(name, ', '). Detect the PDO driver to pick the right syntax. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
description.php: add Google Maps link, current readings table with status indicators, inline 60-day SVG plots for flow/gage/temp. Removes the old plain-text data links row superseded by the richer content. build.py: split index.html into a lightweight state-card grid landing page and move the full all-states table to all.html. Update nav links from /index.html to /all.html for the "All" view. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add running_median() to smooth sparkline data with 3h sliding window - Change merge_sources() to use median for same-timestamp conflicts - Color flow/gage values by section level (low/okay/high from section_level) - Append red "(est)" to names where source uses calc_expression - Gray/italic rows with stale data (>48h); hide expired rows (>7d) - Add CSS classes for level-low, level-okay, level-high, stale, est - Add info_db helpers: is_source_calculated(), classify_level() - Add tests for running_median and median merge behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The observed_at from the database is timezone-naive; normalize it to UTC before comparing with datetime.now(UTC). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Render all date columns and "Updated" timestamps as <time datetime> elements with ISO 8601 UTC values. A small inline JS snippet converts them to the browser's local timezone on page load. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Gauge values: 1 decimal place (was 2) in HTML table - Temperature values: 1 decimal place (was integer) in HTML table - Description page: format flow as integer, gauge/temp as 1 decimal - Description page: add Data Sources section with links to fetch URLs and calculated expressions for all sources linked to the gauge - Description page: render timestamps as <time> for local timezone - Footer: add timezone conversion JS so all PHP pages get local times Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Show gauge location, put-in, and take-out coordinates in decimal degrees when available. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Change "Washington Kayak Club" to "Willamette Kayak and Canoe Club" - In Data Sources, parse calc expression gauge references (e.g. nP::S_Santiam_Cascadia_merge::flow) and link them to the detail page of the section that uses that gauge Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Skip sections with no flow/gauge/temperature data from the main levels table so only sections with active readings are shown - Add a collapsible "All Reaches" directory below the table listing every section alphabetically with links to detail pages - Responsive multi-column layout (2/3/4 columns by screen width) - Fix test mock to include flow data for the filtered table 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>
Reads per-station tables (flow_*, gage_*, temperature_*) from the legacy levels_data MySQL database and upserts into the unified observation table. Supports both SQLite and MySQL targets, with options for time range filtering, dry run, and batch size control. Also syncs the Latest table into latest_observation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The project has been rewritten from C++ CGI to a Python package with a PHP web layer. Update CLAUDE.md with current build commands, test and lint instructions, pipeline architecture, database schema, parser system, and development conventions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reads from levels_todo (metadata), levels_data (observations), and levels_page (page cache) and writes to wkcclevels with the new normalized schema (snake_case columns, string enums). Co-Authored-By: Claude Opus 4.6 <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 24, 2026
mousebrains
added a commit
that referenced
this pull request
May 31, 2026
The PR #99 external verification pass re-confirmed every finding, severity, and the B+ grade against db34ae0 (recommendation: merge), and flagged one inaccurate evidence line plus three off-by-one citations. Corrected: - MED #1: drop the `git branch --contains` "reachable only from main" claim — feature branches later cut from main now contain 9b428bb / 6007c21, so containment no longer distinguishes them. The direct-to- main conclusion stands on the durable evidence (linear f3ed673..HEAD, no merge commit, missing (#NN) suffix). - citations: ci.yml:114→115, SourceUrlTest.php:83-84→84-85, check_reaches.py:212→213. Added an External-review note recording the pass + the one below-LOW item it surfaced (the 0069/0070 header comments' now-stale PENDING_RECONCILIATION wording). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mousebrains
added a commit
that referenced
this pull request
May 31, 2026
* docs: round-6 deep project review (graded B+, ▲ from B−) Sixth deep project review of the entire tracked repo — 6 cold facet auditors (Python, PHP/security, schema/data, tests/CI, ops, docs) + synthesizer hand-re-verification, judging two bands: (A) did round-5's fixes durably stick, and (B) what did #93–#98 + migrations 0069–0071 + the two direct-to-main commits introduce. The recursive integrity check passes cleanly for the first time in the series: every round-5 fix (R1.1/R1.2/R1.3/R1.5/R2.1/R3.x/R4.x) landed as a committed PR and is still present at HEAD, and every mechanized guard is proven non-vacuous by break-it experiment. New code is clean — no CRIT/HIGH: #93 USACE kcfs→cfs (correct, per-series), migrations 0069/0070/0071 (idempotent, FK-clean, Bridgeport DROP cascade residue-free), #96/#97 multi-state pickers, #95/#98 gradient JS. Two MED findings, both recurrences of round-5 classes closed by documentation not mechanization: (1) two direct-to-main commits, one of which broke CI on main (the {}-is-a-dict bug); (2) a nightly snapshot overrode migration 0067's sort_name for gauge 217 with no migration. Root cause is shared — main accepts un-CI-gated direct pushes from both humans and the snapshot bot. Lever: route everything through a CI gate (branch protection + a self-gating/auto-merging snapshot), a snapshot-column drift guard, and teach seed_gauge_display to preserve migration-pinned sort_names. Two facet over-claims dissolved on hand-re-verification (the USACE temperature-docstring drop is a correct fix; check_reaches DOES range-check vertices via validate_lat_lon). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: fold external-review corrections into the round-6 review (#99) The PR #99 external verification pass re-confirmed every finding, severity, and the B+ grade against db34ae0 (recommendation: merge), and flagged one inaccurate evidence line plus three off-by-one citations. Corrected: - MED #1: drop the `git branch --contains` "reachable only from main" claim — feature branches later cut from main now contain 9b428bb / 6007c21, so containment no longer distinguishes them. The direct-to- main conclusion stands on the durable evidence (linear f3ed673..HEAD, no merge commit, missing (#NN) suffix). - citations: ci.yml:114→115, SourceUrlTest.php:83-84→84-85, check_reaches.py:212→213. Added an External-review note recording the pass + the one below-LOW item it surfaced (the 0069/0070 header comments' now-stale PENDING_RECONCILIATION wording). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mousebrains
added a commit
that referenced
this pull request
Jun 2, 2026
Closes residual #1 from the #116 review. The OSM read window was the endpoint bbox, padded by endpoint separation -- which can't cover a reach whose endpoints are close but whose channel bows far laterally (a tight oxbow): `sep` is small, the pad stays at the floor, and the bow clips out of the window -> OSM no-paths -> NHD fallback. `trace_reach` already computes the NHD trace, whose bbox bounds the actual channel for *any* shape. Read OSM within `_coords_bbox(nhd_coords, 0.02deg)` when an NHD trace exists; keep the (adaptive) endpoint `_bbox` only as the fallback when there's no NHD trace. McKenzie still resolves to OSM end-to-end. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mousebrains
added a commit
that referenced
this pull request
Jun 2, 2026
* fix(tracing): address #114 review follow-ups (OSM trace mode) Four non-blocking items from the #114 review; none change the safety invariant (OSM is still gated against NHD, or falls back to it): 1. `_bbox` pad now scales with the endpoint separation (0.05 deg floor) so a reach that bows laterally outside the put-in/take-out box isn't clipped out of the OSM read window (which silently no-paths OSM -> NHD fallback). Short reaches are unchanged -- the floor dominates until ~0.2 deg separation. 2. The NHD-unavailable branch now returns source "osm (ungated)" and logs a WARNING. That's the one path emitting an un-cross-checked OSM geometry; it's now identifiable in the CLI output instead of a bare "osm". 3. `_walk` docstring clarifies the traversable graph spans ALL segments; `pool` only restricts which segments the endpoints snap to. 4. CLI validates `--putin`/`--takeout` are LAT,LON (clean exit 2) rather than a raw IndexError traceback on a missing comma. Adds a `_bbox` adaptive-pad test. pytest 1039 passed; ruff + mypy clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(tracing): bound the OSM read window by the NHD-trace bbox Closes residual #1 from the #116 review. The OSM read window was the endpoint bbox, padded by endpoint separation -- which can't cover a reach whose endpoints are close but whose channel bows far laterally (a tight oxbow): `sep` is small, the pad stays at the floor, and the bow clips out of the window -> OSM no-paths -> NHD fallback. `trace_reach` already computes the NHD trace, whose bbox bounds the actual channel for *any* shape. Read OSM within `_coords_bbox(nhd_coords, 0.02deg)` when an NHD trace exists; keep the (adaptive) endpoint `_bbox` only as the fallback when there's no NHD trace. McKenzie still resolves to OSM end-to-end. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 8, 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>
This was referenced Jun 14, 2026
Merged
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>
This was referenced Jun 22, 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
Complete rewrite of the kayak river levels system from C++ CGI binaries to a Python package (
kayak) with a PHP web layer:Test plan
pytest— 293 tests passruff check src/ tests/— cleanlevels pipeline— full pipeline completes without errorslevels build— generates HTML/CSV/text to public_html/🤖 Generated with Claude Code