…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>
Replaces #4 (auto-closed when its base branch cleanup-and-fixes was deleted on merge of #3).
Summary
Second batch from the deep-review pass. Three small targeted fixes — each one a real correctness or scaling concern surfaced by the original review.
1. FK indexes (migration 0013)
Four FK columns weren't covered by an existing index:
Tables are small today (224 / 1093 / 401 / 461 rows), so latency is fine; the indexes remove the future scaling cliff and match the ix_reach_state_state_id pattern already used for the same reason. Migration applied cleanly on a copy of the prod DB; foreign_key_check + integrity_check both ok; init-db on a fresh DB produces matching indexes.
2. Session factory invalidation (db/engine.py)
get_engine(url=...) disposed the old engine and created a new one, but the cached _session_factory was only rebuilt when get_session_factory() itself was called with a url. A caller going through get_engine() directly left the factory bound to the now-disposed engine — sessions created from it would either fail or write to a stale connection pool kept alive by GC.
Fix: invalidate _session_factory at the same time the old engine is disposed. Regression test exercises the exact path.
3. HTTP connection pooling (utils/http_client.py)
A typical levels fetch run hits ~50 URLs across a handful of hosts. Calling requests.get() opened a fresh TCP+TLS connection per call. Hoisted a module-level requests.Session (lazy via _get_session()) so the second call to a host reuses the pool. reset_session() for test isolation. fetch() now routes through _get_session().get(...).
Test fixtures simplified: per-method @patch("...requests.get") collapsed into one autouse fixture. New TestSessionPooling covers singleton-ness, User-Agent priming, reset behaviour, and shared-session usage.
Test plan
🤖 Generated with Claude Code