Batch 4 part 2 — kayak-deploy: digest-verified paired releases (S7, D2) - #190
Batch 4 part 2 — kayak-deploy: digest-verified paired releases (S7, D2)#190mousebrains merged 14 commits into
Conversation
Sorry, something went wrong.
… activation (S7/4B, D2) The activation orchestrator the plan's S7 calls for, trimmed per decision D2: trust = a full 40-hex commit SHA reachable from each repo's protected branch (verified in a scratch bare clone) plus SHA-256 digests of the built wheel, dataset snapshot, and host-config fingerprint, all recorded in release.json — no signature machinery, and the deployer version is documented rather than negotiated. A release under $KAYAK_DEPLOY_ROOT/releases/<id>/ is self-contained: venv with the engine wheel, read-only dataset snapshot (git archive), contract-validated before anything else, non-secret runtime-config, docroot. The release id derives from wheel digest + dataset SHA + host-config fingerprint, so a host-config-only change is a distinct release. Activation: maintenance flag -> stop consumers -> sqlite .backup -> migrate -> all-or-nothing sync -> build into the release -> atomic symlink switch (ln + rename) -> optional health probe -> restart -> maintenance off; any failure rolls back the symlink, restores the DB backup if mutation began, restarts units, and preserves the scratch backup for manual recovery. --stage-only stops before any system mutation and is both the test mode and the recommended first run on a new host. Tests: ref-shape rejection (short/branch/tag/non-hex), missing-conf failure, unreachable-SHA rejection, and a slow end-to-end staging run (this repo's HEAD as the engine + the fixture dataset as a local git repo) asserting the venv, snapshot, runtime config, and digest manifest. bash-3.2-compatible so the suite runs on the dev Mac; prod needs only python3+pip3+git (hatchling fetched at wheel build). Activation is exercised on a real host by the Batch 4C clean-VM rehearsal (the planned virgin install), which also re-points nginx at $ROOT/current/docroot via the unit/vhost renderers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…detached HEAD git rev-parse --abbrev-ref HEAD returns the literal 'HEAD' on CI's detached checkout, so ENGINE_BRANCH=HEAD made the deployer's single-branch clone fail (exit 128) in both branch-dependent tests. The module fixture now pushes HEAD into a scratch bare repo under a known 'test-main' branch, which works identically attached or detached. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…kout is shallow too git push from a shallow repository is refused, so the bare-repo push fixture failed on CI just like --abbrev-ref had. git archive HEAD works regardless of shallow/detached state; the staging test exercises the deployer's mechanics (clone, wheel build, snapshot, digests), not provenance, so a tree-identical single-commit repo is the right fixture. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…umed Third CI-environment difference: the test job's PATH leads with a uv-managed venv whose python has no pip module, so 'python3 -m pip wheel' failed. The deployer now needs only the stdlib venv/ensurepip: a scratch venv supplies pip for the wheel build and the release venv bootstraps its own, with an early capability check (clear error + KAYAK_DEPLOY_PYTHON escape hatch). Host requirement documented: python3-venv on Debian, no system pip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Adversarial review findings for PR #190 at current head 1c7d7cb26bde57835eb05db4d434b6cba154d0b0:
Verification run locally on the current PR head:
|
Sorry, something went wrong.
… identity, locked deps, virgin rollback All five PR #190 findings: P1 sidecars: new packaged kayak.db.sidecars + 'levels import-metadata' (the geom/gradient apply sync-metadata deliberately excludes; scripts/import_metadata.py now delegates to the same functions); activation runs it after sync, before build, so a sidecar-only dataset release reaches the DB. db/sidecars.py joins the writer-boundary ALLOWLIST with rationale. P1 quiesce: activation stops timers AND their services, then waits (max 120s) for the service set to drain before the pre-mutation backup — a timer stop alone leaves an in-flight oneshot writing under the backup/ migrate/sync. P1 identity: the non-secret runtime config is emitted during staging from the live host env and its digest joins the release id, so an /etc/kayak/env change mints a NEW release instead of reusing a stale runtime-config/docroot. Manifest records it. P2 locked deps: the engine commit now carries requirements-prod.lock (hash-pinned uv export, drift-checked in CI); release and staging venvs install with --require-hashes + the wheel with --no-deps, and the lock digest joins the release id — the same engine SHA installs identical bytes on any day. P2 virgin rollback: the symlink switch is tracked; a first-activation failure removes 'current', keeps maintenance mode, and does NOT restart consumers (there is no prior release to serve). Tests: import-metadata unit suite (both/only/rollback/partial/ malformed); staging test asserts the new digests and that a SITE_URL change yields a different release id. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
All five findings addressed in 6f746c7: P1 — sidecar apply: new packaged kayak.db.sidecars + levels import-metadata command (default applies both sidecars; --geom-only/--gradient-only are a mutually-exclusive group; unmatched ids roll back and fail unless --allow-missing-reaches). scripts/import_metadata.py now delegates to the same packaged functions — one implementation. Activation runs it between sync-metadata and build, covered by the pre-activation DB backup. db/sidecars.py joins the writer-boundary ALLOWLIST with rationale. P1 — quiesce: activation stops timers and their corresponding services, then waits (≤120 s) for the whole service set to drain before the backup; a still-active service aborts rather than mutating under it. P1 — release identity: staging now installs the engine into the scratch toolchain, validates the dataset, and emits the non-secret runtime config from the live host env before the release id is derived — the config digest (and the dependency-lock digest) join the id and the manifest, so an /etc/kayak/env change mints a new release; stale-reuse is structurally impossible. P2 — locked deps: the engine now carries requirements-prod.lock (hash-pinned uv export, drift-checked in CI against uv.lock); both staging and release venvs install with --require-hashes plus the wheel --no-deps. Same engine SHA ⇒ identical dependency bytes, any day, recorded in the manifest. P2 — virgin rollback: the symlink switch is tracked; a first-activation failure removes current, keeps maintenance mode, and does not restart consumers — a half-activated first install neither serves nor writes. Tests: new import-metadata unit suite (both/only/rollback/partial/malformed), and the staging test now asserts the lock+config digests and that a SITE_URL change yields a different release id. Full gate green: pytest 1723 + the slow staging run (now two stagings, ~83 s), mypy, shellcheck, wheel-smoke, lock-drift check in sync. |
Sorry, something went wrong.
|
Re-review of PR #190 at current head 6f746c7ccff18764e3036c74ace913488aa2410f after the previous fixes:
Validation I ran locally:
|
Sorry, something went wrong.
|
Adversarial review for updated PR #190 at head 6f746c7ccff18764e3036c74ace913488aa2410f, with live-server context included. Live context I checked:
Findings:
Verification performed on the current PR head:
|
Sorry, something went wrong.
…ty, locked build backend, virgin pre-switch rollback Second-round PR #190 findings: P1 serving gap: activation now REFUSES unless deploy.env sets SERVING_CUTOVER=yes — the flag the Batch 4C install/migration runbook sets when nginx root + FPM open_basedir actually point at $ROOT/current. Until then stage-only works and scripts/deploy.sh stays the deploy path; the deployer can no longer report success while users are served the legacy docroot. P1 config activation + secret boundary: activation installs the canonical /etc/kayak/runtime-config.json exactly the way deploy.sh does — emit-config --dry-run piped into the root kayak-install-runtime-config wrapper (root-only secrets merged, 0640 root:www-data), emitted with FINAL release paths. P1 identity stability + token retention: the release id's config digest is computed over a NORMALIZED view (staging-local path fields and ntfy/hc_* operational tokens excluded, keys sorted), so identical inputs mint the identical release id, the release-retained copy holds no dead scratch paths and no notification/healthcheck tokens, and the canonical secret-merged config lives only under /etc/kayak. P2 build backend: requirements-build.lock (hash-pinned hatchling chain via uv pip compile) is preinstalled into the build venv and the wheel builds with --no-build-isolation — no network resolution and no unpinned backend code at deploy time; its digest joins the release id and manifest. P2 virgin rollback: with no previous release, ANY activation failure — before or after the symlink switch — now leaves the host in maintenance with consumers stopped ('current' removed only if the switch happened). Tests: same-input staging mints the same release id (reuse path), the retained runtime-config has no scratch paths / no ntfy_topic / no hc_* / no token value, and the SITE_URL-change different-id assertion stays. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Zip entries carry checkout mtimes, so the same engine SHA hashed differently per clone — breaking the same-inputs => same-release-id property the re-review demanded. hatchling honors SOURCE_DATE_EPOCH; pin it to the commit's own timestamp (deterministic per SHA). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Its engine default is BASE_DIR-relative, which resolves inside the scratch build venv — the one remaining nondeterministic field in the release-identity digest (and a dead path in the retained copy). The DB location is host-runtime shape; PHP reads it from the canonical wrapper-installed /etc/kayak/runtime-config.json. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
All seven findings from both second-round passes addressed (e8f04f0): P1 — serving gap: activation now refuses unless deploy.env sets SERVING_CUTOVER=yes — the flag the Batch 4C install/migration runbook sets when nginx root + FPM open_basedir actually point at $ROOT/current. Until then --stage-only works and scripts/deploy.sh remains the deploy path; the deployer can no longer report success while users are served the legacy docroot. P1 — config activation + secret boundary: activation installs the canonical /etc/kayak/runtime-config.json exactly the way deploy.sh does — emit-config --dry-run piped into the root kayak-install-runtime-config wrapper (root-only secret merge, 0640 root:www-data), emitted with final release paths. P1 — identity stability + token retention: the release id's config digest is computed over a normalized view — staging-local path fields (dataset_dir, output_dir, database_url, caches) and operational tokens (ntfy_topic, hc_*) excluded, keys sorted. Two more nondeterminism sources fell out of testing this properly: wheel zip mtimes (fixed with SOURCE_DATE_EPOCH = the commit's own timestamp, honored by hatchling) and the BASE_DIR-relative database_url default resolving inside the scratch venv. The staging test now stages three times: same inputs ⇒ same release id (reuse path hit), SITE_URL change ⇒ new id, and the retained config is asserted to contain no scratch paths, no ntfy_topic/hc_* keys, and not the seeded token value. P2 — build backend: new requirements-build.lock (hash-pinned hatchling chain via uv pip compile --generate-hashes) preinstalled into the build venv; the wheel builds with --no-build-isolation — no network resolution and no unpinned backend code at deploy time; its digest joins the release id and manifest. P2 — virgin rollback (pre-switch): with no previous release, any activation failure — before or after the symlink switch — now leaves the host in maintenance with consumers stopped (current removed only if the switch happened). Gate: pytest 1723 + the slow triple-staging run (~2 min), mypy, ruff, shellcheck — green. |
Sorry, something went wrong.
|
Re-review at e8f04f0 after the latest updates. CI is green now. Findings:
Local validation run: bash -n deploy/kayak-deploy.sh, git diff --check origin/main...HEAD, focused ruff check, and focused uv run --extra dev pytest -q tests/test_cli/test_import_metadata.py tests/test_scripts/test_kayak_deploy.py --override-ini=addopts=. The focused pytest run passed (9 passed). |
Sorry, something went wrong.
|
Adversarial re-review for PR #190 at head e8f04f0a8d783ffd5fb9d8db1eb0d69434a892f3, including live-server context. CI was still running when requested; by my final check all GitHub checks were green for this head. Live context checked:
Findings:
Validation performed locally on this head:
|
Sorry, something went wrong.
…-lock CI check, activation test PR #190 third-round findings (both passes): P1 config rollback: activation snapshots /etc/kayak/runtime-config.json before installing the new one and rollback() restores it on any post-install failure (failed switch/health), so a rollback never leaves the old release running with the failed release's config. P1 secret retention: the normalized release-retained config now drops any secret/password/token-shaped field (root-run staging emit unwraps SecretStr like turnstile_secret) in addition to path/ntfy/hc_* fields. P1 privilege model: one orchestrator — root for systemctl + the config wrapper; DB/build steps (backup/migrate/sync/import/build) run as KAYAK_APP_USER via runuser so root never creates app-owned-resource WAL sidecars. Read-only scratch staging stays as the orchestrator (the secret filter, not the uid, guards the retained copy). Required-when-root KAYAK_APP_USER documented; the root/runuser env propagation is validated in the 4C clean-VM rehearsal. P2 build-lock drift: requirements-build.lock is regenerated from pyproject.toml [build-system].requires and diffed in CI, so a backend change can't merge stale and only fail at deploy time. Tests: systemctl + config-installer are parameterized so a new slow activation test runs the full path unprivileged — first release activates, a second release with a failing health check rolls back the symlink, DB, AND runtime config to release 1. The staging test now also asserts TURNSTILE_SECRET never reaches the retained config. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
All third-round findings addressed in 65b7f29 (both passes): P1 — config rollback (both reviewers): activation snapshots /etc/kayak/runtime-config.json before installing the new one, and rollback() restores it on any post-install failure (failed ln/mv/health). A rollback now returns symlink, DB, and runtime config to the previous release together. New slow activation test proves it: first release activates, a second release with a failing health probe rolls all three back to release 1. P1 — secret retention: the normalized release-retained config drops any secret/password/token-shaped field (a root-run staging emit unwraps SecretStr like turnstile_secret), on top of the path/ntfy/hc_* filter. The staging test now seeds TURNSTILE_SECRET and asserts neither the key nor its value survives. P1 — privilege model: one orchestrator. Root performs systemctl + the root config wrapper; every step that writes persistent app-owned state (backup/migrate/sync/import/build) runs as KAYAK_APP_USER via runuser, so root never creates app-owned-resource WAL sidecars. Read-only scratch staging stays as the orchestrator (the secret filter, not the uid, guards the retained copy). KAYAK_APP_USER is required-when-root and documented; the root/runuser env propagation is validated in the 4C clean-VM rehearsal (it needs real root+systemd — same framing as the rest of the activation phase). P2 — build-lock drift: requirements-build.lock is now regenerated from pyproject.toml [build-system].requires and diffed in CI, so a backend change can't merge stale and only surface at deploy time. Gate: pytest 1723 + the slow deploy suite (10 tests, ~3.5 min incl. the new activation rollback test), mypy, ruff, shellcheck, both lock drift checks — green. |
Sorry, something went wrong.
|
Re-review at 65b7f2959328ffa0b02ec591efcf0418f7776e84 after the latest updates. CI is green by final check. Findings:
Local validation run on this head: bash -n deploy/kayak-deploy.sh, git diff --check origin/main...HEAD, targeted ruff check, targeted mypy, shellcheck deploy/kayak-deploy.sh (only the expected dynamic /etc/kayak/env SC1091), and focused uv run --extra dev pytest -q tests/test_cli/test_import_metadata.py tests/test_scripts/test_kayak_deploy.py --override-ini=addopts=. The focused pytest run passed: 10 passed in 233.51s. |
Sorry, something went wrong.
|
Adversarial re-review for PR #190 at head 65b7f2959328ffa0b02ec591efcf0418f7776e84, including live-server context. CI was still running when requested; by my final check all GitHub checks were green for this head. Live context checked:
Findings:
Validation performed locally on this head:
|
Sorry, something went wrong.
…ase verify (4th round) PR #190 fourth-round findings (both passes): P1 app-user DB boundary: the pre-activation backup now lands in an APP-OWNED scratch dir (runuser mktemp -d), since the orchestrator's 0700 mktemp -d is untraversable by the app user; the rollback DB restore runs through run_app too, so neither path recreates the root-owned WAL/SHM footgun. run_app's privilege decision and the runuser command are overridable (KAYAK_PRIVILEGED / KAYAK_RUNUSER) so the activation test exercises the privileged branch with a same-user shim — it now asserts backup/migrate/sync/import/build AND the rollback restore all cross the app-user boundary. P1 consumer cutover: SERVING_CUTOVER now means the whole host is cut over, and activation verifies each levels-running consumer unit's ExecStart points at $ROOT/current before mutating — otherwise the next pipeline/decimate/status run would execute the old checkout against the migrated DB. Gate doc + deploy.env.example updated. P2 reused-release verify: a reused release dir is now fully verified — every digestable retained artifact (wheel, prod lock, runtime-config) against both the manifest and the recomputed inputs, the dataset tar digest, and a venv liveness check; the dataset is re-extracted from the freshly-verified tar. Any mismatch fails closed (test corrupts a retained lock and asserts refusal). Also fixes an EXIT-trap gotcha the new app-scratch introduced: cleanup ended on a failing test expression, leaking exit 1 into stage-only (caught by the staging test). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
All fourth-round findings addressed in 97a2987 (both passes): P1 — app-user DB boundary (real bug): the pre-activation backup now lands in an app-owned scratch dir (runuser mktemp -d), since the orchestrator's mktemp -d is 0700 root — untraversable by the app user, so the previous $SCRATCH/app approach would have failed at the first backup on a real root run. The rollback DB .restore runs through run_app too, so neither the forward nor the failure path recreates the root-owned WAL/SHM footgun. run_app's privilege decision and the runuser command are now overridable (KAYAK_PRIVILEGED/KAYAK_RUNUSER), so the activation test runs the privileged branch with a same-user shim and asserts backup/migrate/sync/import/build and the rollback restore all cross the boundary. P1 — consumer cutover: SERVING_CUTOVER now means the whole host is cut over, and activation verifies each levels-running consumer unit's ExecStart points at $ROOT/current before mutating — closing the gap where the next pipeline/decimate/status run would execute the old checkout against the migrated DB. Gate doc + deploy.env.example updated. P2 — reused-release verify: a reused release dir is now fully verified — every digestable retained artifact (wheel, prod lock, runtime-config) against both the manifest and the recomputed inputs, plus the dataset-tar digest and a venv liveness check; the dataset is re-extracted from the freshly-verified tar. Any mismatch fails closed (new test corrupts a retained lock and asserts refusal). Bonus the new tests caught: adding the app-scratch introduced an EXIT-trap gotcha (cleanup ended on a failing [ … ] chain, leaking exit 1 into stage-only) — fixed. Gate: pytest 1723 + the full deploy suite (11 tests now, incl. privileged-mode activation rollback + corrupted-reuse refusal, ~4 min), mypy, ruff, shellcheck, both lock drift checks — green. (Local runs need TMPDIR off the 964 MB /tmp tmpfs, as you noted.) |
Sorry, something went wrong.
Adversarial review — PR #190 (kayak-deploy, S7/D2)Reviewed against the live host's actual configuration, not just the diff. The design is sound and the engineering is careful (atomic switch, app-user DB boundary, hash-locked deps, fail-closed reuse, reproducible wheels). The PR is also genuinely inert today — /usr/local/sbin/kayak-deploy is not installed, there's no /etc/kayak/deploy.env, /opt/kayak doesn't exist, and every consumer unit still runs /home/pat/.venv/bin/levels. So no regression risk to current prod. The findings below are about what happens when 4C actually points this at the live host. The recurring theme: the tests pass only because they inject into the process env the very values the live host does not supply that way, so the staging/activation env model is effectively unverified against reality. P1 — SQLITE_PATH won't resolve on this host → activation aborts before any workkayak-deploy.sh:85-90 sources only /etc/kayak/env, with the comment "The standard host environment (SITE_URL, SQLITE_PATH, …)". On this host that file contains exactly one line: KAYAK_HOME=/home/pat. The real config — SQLITE_PATH, SITE_URL, DATASET_DIR, OUTPUT_DIR, NTFY_TOPIC, HC_* — lives in /home/pat/.config/kayak/.env (0600 pat). The systemd units get it via a second EnvironmentFile=/home/pat/.config/kayak/.env (confirmed in kayak-pipeline.service); the deployer never reads that file. Consequence at kayak-deploy.sh:462-466: DB_PATH="${SQLITE_PATH:-}"
if [ -z "$DB_PATH" ] && [ -r /etc/kayak/env ]; then
DB_PATH="$(. /etc/kayak/env >/dev/null 2>&1; echo "${SQLITE_PATH:-}")"
fi
: "${DB_PATH:?SQLITE_PATH must be set (env or /etc/kayak/env) for activation}"SQLITE_PATH is in neither the orchestrator env nor /etc/kayak/env, so DB_PATH is empty and activation aborts at the :? guard. test_activation_* only passes because it sets SQLITE_PATH directly in the subprocess env. Fix options: have the deployer also source ~${KAYAK_APP_USER}/.config/kayak/.env, or have the 4C runbook consolidate SQLITE_PATH/DATASET_DIR/SITE_URL into /etc/kayak/env (and say so in deploy.env.example). Either way the comment at line 85 is currently false for this host. P1 — staging emit-config (root) needs SITE_URL, which only arrives via the sudo/SUDO_USER pathThe live dataset is status: publishable (/home/pat/kayak_data/dataset.yaml), so require_explicit_site_url_for_publishable_dataset makes emit-config fail unless SITE_URL is in os.environ (config.py:330-332, site_url_is_explicitly_configured() reads os.environ). Staging runs emit-config as the orchestrator (kayak-deploy.sh:281, not run_app), i.e. root on a real host. This works only if the deployer is invoked as sudo kayak-deploy from pat — then SUDO_USER=pat triggers config.py:43-51's fallback to /home/pat/.config/kayak/.env, and load_dotenv puts SITE_URL into os.environ. If kayak-deploy is ever started directly as root (root login, systemd, cron — none of which set SUDO_USER), the fallback can't fire, SITE_URL is absent, and even --stage-only fails for the publishable dataset. The test deliberately sets SUDO_USER="" and instead injects SITE_URL, so this invocation contract is both undocumented and untested. Please document "must be run via sudo from the app user" (or pass SITE_URL explicitly) and ideally assert it early. P2 — cutover-verification gate's *levels* heuristic misses an engine consumerThe gate (kayak-deploy.sh:440) only verifies units whose ExecStart contains levels: case "$es" in *levels*) … must contain $ROOT/current … ;; esacBut kayak-audit-gauges.service — which is in the default KAYAK_UNITS — runs /home/pat/.venv/bin/python3 .../scripts/audit_gauges.py (no levels token), so it gets stopped/started but its ExecStart is never checked against $ROOT/current. After 4C it could keep running the old checkout against the freshly-migrated DB and the gate would still pass. (It's read-mostly, so low blast radius — but the heuristic is a leaky proxy for "engine consumer.") Same class: kayak-recap/-healthcheck/-heartbeat are shell scripts under /home/pat/kayak/ and aren't in KAYAK_UNITS at all. Consider keying off $ROOT/current presence or an explicit host-script exemption list rather than the levels substring. P2 — no release pruning; unbounded disk growth/opt/kayak/releases/<id>/ each carries a full venv. Nothing GCs old releases (the only rm -rf paths are scratch and a restage's dataset/). On the Hetzner VPS this accumulates a venv per deploy with no retention bound. Worth a "keep last N" sweep before this becomes the deploy path — and given rollback relies on PREV_TARGET, you need at least the previous release retained anyway. P2 — activation test fidelitytest_activation_rolls_back_* stubs runuser with a same-user, env-preserving shim (shift 3; exec "$@"), points HOME at tmp_path, and injects SITE_URL/SQLITE_PATH. That means three real-host behaviors are entirely unexercised: (a) real runuser -u env semantics, (b) config resolution via ~pat/.config/kayak/.env + the SUDO_USER fallback, (c) the bash SQLITE_PATH/SITE_URL sourcing gap (P1 above). The PR acknowledges (a) is deferred to the 4C clean-VM rehearsal; please make that rehearsal explicitly assert (b) and (c) too, since they're the latent failures here. Verified solid (credit where due)
Nit
Bottom line: mergeable as inert infrastructure, but the two P1s must be reconciled in the 4C runbook before this orchestrator can stage or activate on the live host as currently configured — the config lives in ~pat/.config/kayak/.env, and the deployer's shell only looks at /etc/kayak/env. — reviewed on the live server (levels.wkcc.org) |
Sorry, something went wrong.
…, type-based secret exclusion PR #190 live-host review (reviewed against the running server): P1 host-env model: on the WKCC host /etc/kayak/env holds only KAYAK_HOME; the real config (SQLITE_PATH/SITE_URL/DATASET_DIR) lives in the app user's ~/.config/kayak/.env, which the units load via a second EnvironmentFile=. The deployer now sources BOTH (KAYAK_HOST_ENV + KAYAK_APP_ENV/the app user's .config, resolved via getent) with set -a so subprocesses inherit them — so staging emit-config sees SITE_URL and DB_PATH resolves, run directly as root or via sudo (no SUDO_USER dependency). The false 'standard host environment' comment is fixed; SQLITE_PATH/SITE_URL absence fails early and clearly. P2 cutover gate: replaced the leaky *levels* substring (which missed kayak-audit-gauges, a python-run engine consumer) with: every consumer service must run from $ROOT/current unless explicitly in KAYAK_HOST_UNITS (default: the backup units). P2 release pruning: keep KAYAK_KEEP_RELEASES (default 5) most-recent releases after a successful activation, always retaining current + previous (rollback needs PREV_TARGET); each release carries a full venv so this bounds VPS disk growth. Secret exclusion: emit-config gains --exclude-secrets (TYPE-based: drops every SecretStr field, not a name heuristic), which the deployer uses for the normalized/retained config so a future secret field can't leak by name; the name filter stays as a second line of defense. Nit: the fetch_and_verify comment no longer claims a non-existent explicit fetch. Tests: the activation test now resolves SITE_URL+SQLITE_PATH from a KAYAK_HOST_ENV FILE (exercising the real sourcing the live review flagged as untested), new prune test (3 activations, KEEP=0, asserts the oldest pruned + current/previous kept), and an emit-config --exclude-secrets type-based test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The release-activation cutover used the idiom
ln -s "releases/$RELEASE_ID" "$ROOT/current.new"
mv -f "$ROOT/current.new" "$ROOT/current"
which is correct only on the FIRST activation, when $ROOT/current does
not yet exist. On every later deploy `current` is a symlink to a
directory, and both GNU and BSD `mv` dereference such a destination:
`mv -f current.new current` moves current.new INTO releases/<old-id>/,
leaving `current` pointed at the OLD release. Build/migrate/sync/
config-install all succeed and the script exits 0 — but nothing serves
the new release. The new prune step then reads `cur` from the stale
symlink, so the freshly-built (now unreferenced) release matches neither
current nor previous and is garbage-collected: a no-op cutover that also
deletes the release it was meant to activate.
Replace both the forward switch and the rollback restore with an
`atomic_relink` helper that repoints the symlink WITHOUT dereferencing
it: GNU `mv -T`/--no-target-directory, BSD/macOS `mv -h`, each an atomic
rename(2). Try GNU, then BSD, then a non-atomic remove+move only if `mv`
supports neither flag.
Caught by test_activation_prunes_old_releases (added with the pruning
feature): three real activations produce three distinct release ids, yet
`current` stayed pinned to release 1 and only release 1 survived on disk.
Now green, alongside the existing rollback activation test (which also
performs a second switch over an existing `current`).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The prod host's /tmp is a ~1 GB tmpfs. kayak-deploy staged into `mktemp -d` (i.e. /tmp): the wheel build + two venv installs (a few hundred MB) plus the app-owned pre-activation DB backup (the live DB is ~650 MB) overflow it, and the deploy dies mid-stage with ENOSPC — exactly when it is about to mutate production. No unit test could surface this; it took the Batch 4C clean-VM rehearsal (the test VM mirrors prod's small /tmp tmpfs). Default the scratch base to KAYAK_DEPLOY_TMPDIR (= $KAYAK_DEPLOY_ROOT/.staging, always real disk) for BOTH the orchestrator scratch and the app-owned DB-backup scratch. The app scratch is carved from the same real-disk base (0755 so the app user can traverse in) and chowned to the app user, instead of the app user's own `mktemp -d`. Use the portable `mktemp -d <dir>/X...` template form (`-p` is GNU-only) so the macOS unit tests still pass. Validated on the clean-VM clone: stage-only AND a full activation (the 650 MB DB backup) both succeed with /tmp at 82% and no TMPDIR set; /tmp usage stays flat. deploy.env.example documents the var and the sourced-shell quoting rule for multi-word values. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
What's new since the last review round + how it was validated on real hardwareHead is now a22585b. Three commits landed since the last reviewed state (97a2987), and — more importantly — the whole deployer was exercised end-to-end on a faithful copy of the production host, which is where the two bug fixes below came from. None of that validation is visible in the diff, hence this note. The clean-VM rehearsal (the main new evidence)A throwaway VM was built into a faithful current-live copy (same Debian 13.5, same packages, the live DB copy — 432 reaches, the two repos at the live SHAs, real nginx + PHP-FPM + the 14 systemd units serving the site over HTTPS). Then the full live → paired-release cutover was run on it via kayak-deploy:
This is real root + real systemd + a real second service user + GNU coreutils — the things the unit tests could only simulate. Two bugs the rehearsal found (and fixed)abb8ded — the atomic symlink cutover silently no-ops on every deploy after the first. ln -s … current.new && mv -f current.new current is correct only when current doesn't exist yet. Once current is a symlink to a directory, both GNU and BSD mv dereference it and move current.new into the old release dir, leaving current on the old release — then the new prune step reads cur from the stale symlink and GCs the freshly-built release. (This idiom was marked "Verified solid" in a prior round; it isn't.) Fixed with an atomic_relink helper using the portable "don't treat dest as a directory" flag — GNU mv -T / BSD mv -h, each an atomic rename(2) — used by both the forward switch and the rollback restore. Validated by the second-activation row above (and it confirms the bug would have hit prod, since the clone is GNU coreutils). a22585b — staging into /tmp overflows the prod tmpfs (ENOSPC mid-deploy). Prod's /tmp is a ~1 GB tmpfs. The deployer staged into mktemp -d (i.e. /tmp); the wheel build + two venv installs plus the app-owned ~650 MB pre-activation DB backup overflow it, and the deploy dies right as it's about to mutate prod. This would have failed the real cutover. Fixed: KAYAK_DEPLOY_TMPDIR (default $KAYAK_DEPLOY_ROOT/.staging, always real disk) for both the orchestrator scratch and the app-owned DB-backup scratch. Validated on the clone — stage-only and a full activation (the 650 MB backup) both succeed with /tmp at 82% and no TMPDIR set; /tmp usage stays flat. be43a93 is the response to the previous live-host review (app-user ~/.config/kayak/.env sourcing so SITE_URL/SQLITE_PATH reach emit-config; cutover gate keying off $ROOT/current rather than the levels substring; prune_releases + KAYAK_KEEP_RELEASES; type-based --exclude-secrets). Findings deferred to follow-on PRs (not in this PR)
The cutover itself is still gated behind SERVING_CUTOVER=yes, so this remains inert on the live host until the 4C runbook flips it. |
Sorry, something went wrong.
|
Adversarial review, including the current live-server context: FindingsP1 - Root deployer executes the app user's dotenv as shell deploy/kayak-deploy.sh:97-116 sources both KAYAK_HOST_ENV and the app user's ~/.config/kayak/.env with set -a in the deployer process. On the live server, the second file is exactly what the units load (EnvironmentFile=-/home/pat/.config/kayak/.env) and it is app-user owned (-rw------- pat pat). systemd treats EnvironmentFile= as data; . "$APP_ENV" is shell execution. In the normal root activation path, any app-user write to that file can execute arbitrary commands as root before the deployer reaches the trust/ref checks, and can also override deploy controls such as ENGINE_REPO, KAYAK_SYSTEMCTL, KAYAK_CONFIG_INSTALLER, KAYAK_UNITS, etc. This turns the service account's runtime config file into a root code-execution surface. Please parse the needed runtime keys as data with a narrow allowlist, or run the env-loading/config-emission portion entirely under the app user and pass only sanitized values back. Do not source an app-owned dotenv in the root orchestrator. P1 - The default quiesce/cutover unit set misses a live DB consumer The live host currently has kayak-healthcheck.timer active, and kayak-healthcheck.service runs /home/pat/kayak/scripts/health-check.sh with ReadWritePaths=/home/pat/DB. That is a DB consumer and, after 4C, also a release-layout consumer. But the deployer default at deploy/kayak-deploy.sh:123 and the example at deploy/deploy.env.example:22 omit kayak-healthcheck.timer. The cutover gate and quiesce loops only inspect $KAYAK_UNITS (deploy/kayak-deploy.sh:500-521, 607-628), so with the shipped defaults the deployer can migrate/sync/restore the DB while the hourly healthcheck remains runnable from the legacy checkout, and it will not fail the "all consumers point at $ROOT/current" gate. This is not just theoretical: systemd/install.service.sh installs kayak-healthcheck.timer, deploy/SETUP.md documents it, and it is active on this server. Either derive the set from installed/active kayak-* timers and require explicit host-unit exemptions, or make the default/example complete for every DB/release consumer. At minimum, kayak-healthcheck.timer needs to be in the default stop/start/gate set. P2 - Reusing an existing release mutates the release before stage-only/gates/quiesce In the reuse branch, deploy/kayak-deploy.sh:386-395 does rm -rf "$RELEASE_DIR/dataset" and re-extracts the dataset before the --stage-only exit, before SERVING_CUTOVER is checked, and before consumers are stopped. If $RELEASE_DIR is the current release, a harmless-looking stage-only rehearsal with the same refs can temporarily remove the live release's dataset while timers/web/manual consumers are still running. That violates the "stage-only stops before system mutation" promise and the release immutability model. Reuse should be read-only until activation quiesces consumers. Verify against scratch artifacts without deleting the existing release's dataset, or restage into a new temp dir and atomically replace only when it is safe. Verification
|
Sorry, something went wrong.
Adversarial review — PR #190 (kayak-deploy paired-release orchestrator)Reviewed deploy/kayak-deploy.sh + supporting Python against the running WKCC host (levels, the live editable-install tree). This PR has already been through ~6 review rounds; I focused on what only a check against the real production environment can surface. Verdict: no merge blockers. The activation path is inert today (refused unless SERVING_CUTOVER=yes, which no host sets yet), so this lands safely. Everything below is hardening that bites at or after the Batch 4C cutover, when this script first mutates prod for real. What I verified against the live host (claims that hold up ✅)
FindingsF1 — Medium (defense-in-depth gap): the default KAYAK_UNITS / cutover gate is blind to several live DB-reading & engine consumers.
The cutover-verification loop iterates only over $KAYAK_UNITS, so it can't catch a consumer you forgot to list. Two consequences: (a) health-check/heartbeat read the DB concurrently with migrate/sync-metadata — I confirmed both are read-only (no INSERT/UPDATE/DELETE), so no corruption (SQLite WAL), but a destructive migration mid-deploy can fire a spurious health-check failure alert; (b) more importantly, the gate's own stated purpose — "the next pipeline/decimate/status run would execute the old checkout against the migrated DB" — is exactly what kayak-recap would do after a cutover, undetected, because it isn't on the list. Recommend the default KAYAK_UNITS (and deploy.env.example) enumerate every engine/DB-touching unit, or derive the consumer set instead of hardcoding; at minimum make the 4C runbook responsible for a complete list. (KAYAK_UNITS is overridable, so this is about the default being a trustworthy safety net.) F2 — Low (latent, conditional on root umask): the umask-077 defense is incomplete. F3 — Low (divergent-parser footgun): the deployer shell-sources the app user's ~/.config/kayak/.env. F4 — Nit: the host-config fingerprint is inert on this host. /etc/kayak/host.yaml doesn't exist, so HOST_FP="none" always. The headline "a host-config-only change is a distinct release" property is dormant until host.yaml is actually deployed (PR #189 added the typed config; the file isn't installed). Handled gracefully — just don't assume that identity guarantee is live yet. F5 — Nit: HEALTH_URL is optional. With it unset, activation flips the symlink with no post-switch health gate (the rollback-on-failed-health path never runs). deploy.env.example suggests https://levels.wkcc.org/status.json — recommend the 4C runbook treat it as required for the first real cutover. F6 — Nit/operational: a deploy overlapping the hourly *:12 pipeline terminates the in-flight run. Pipeline runs measure ~110–116 s steadily on this host; kayak-pipeline.service is Type=oneshot, KillMode=control-group, TimeoutStopUSec=90 s. So systemctl stop mid-run kills the cycle (SIGTERM→SIGKILL ≤90 s; data-safe under WAL), and the 120 s drain backstop sits just above the 90 s stop timeout — sized fine. Just schedule deploys clear of the :12 window so you don't routinely abort a running fetch. Bottom line: solid, genuinely well-hardened work — the inert-until-cutover gating means this merges with no risk to the live site. F1 is the one I'd want addressed (or explicitly delegated to the 4C runbook) before the first real activation, since the consumer set is what stands between a migrate/sync and old code reading the post-migration DB. F2–F3 are latent footguns worth a cheap fix; F4–F6 are notes for the cutover runbook. Reviewed against the live WKCC host on 2026-06-13. 🤖 Generated with Claude Code |
Sorry, something went wrong.
…k, units) Two live-server adversarial reviews of a22585b (codex/gpt-5.5 + claude/ opus). Both: no merge blocker (inert until SERVING_CUTOVER=yes). These are the real code-level findings, fixed before merge: - SECURITY (codex P1 / claude F3): the deployer no longer shell-`source`s the app-user-owned ~/.config/kayak/.env as root — that was a root code-execution + deploy-control-override surface (it could set ENGINE_REPO, KAYAK_SYSTEMCTL, …) introduced in be43a93. Read only the allowlisted data keys (SITE_URL/SQLITE_PATH/DATASET_DIR/OUTPUT_DIR) as data, only when the root-owned env hasn't already set them. /etc/kayak/ env (root-owned) is still sourced. - reuse READ-ONLY (codex P2): the reuse branch no longer rm's + re-extracts the (possibly live) release's dataset before the stage-only exit / quiesce — it diff-verifies the on-disk dataset against the tar from scratch and fails closed on mismatch. Upholds "stage-only stops before mutation." - umask 022 (claude F2): consistent dir perms so $ROOT/releases/<id> stays app-user-traversable even under a root umask of 077 (the .staging chmod alone was inconsistent). - kayak-healthcheck.timer added to the default KAYAK_UNITS + example (a live DB consumer the cutover gate couldn't see); the 4C runbook owns the COMPLETE enumeration. Tests: new test_app_env_keys_read_as_data_not_sourced (a malicious app .env line must not execute, the allowlisted key is read as data, a non-allowlisted deploy control can't override); a reuse-is-read-only assertion; and the slow activation tests stage on a real-disk root (/var/tmp) so the module is runnable on a prod-shaped host (964 MB /tmp tmpfs) — codex couldn't run it before. `_run` gained None=remove-key for hermetic envs. Deferred to the 4C runbook (cutover-time): the robust consumer-set derivation (recap/heartbeat/config-drift), HEALTH_URL required for the first cutover, and scheduling clear of the :12 pipeline. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Both live-server reviews addressed in 530b4a0. Both concluded "no merge blocker" (inert until SERVING_CUTOVER=yes); the items below are the real code-level ones, fixed before merge — the rest are cutover-time and go to the 4C runbook. Fixed in 530b4a0
Deferred to the 4C runbook / PR (cutover-time, as both reviews suggested)
Gate: ruff/format/shellcheck/mypy clean; 1725 fast tests + the slow deploy suite (stage/rollback/prune on a real-disk root) green. |
Sorry, something went wrong.
Re-review of 530b4a0 — fixes verified against the live hostChecked the hardening commit against the running WKCC host. All four changes hold up, and the one finding that mattered is fully closed. ✅ F3 / codex P1 (the security one) — closed and verified RCE-safe. Replacing the . "$APP_ENV" shell-source with the allowlisted data-reader was the right call (and codex's escalation to a root code-exec / deploy-control-override surface is correct — a source of the app-user-owned file as root could set ENGINE_REPO, KAYAK_SYSTEMCTL, etc.). I exercised the new loop adversarially:
✅ F2 (umask) — fixed correctly. umask 022 up front makes $ROOT/releases/<id> 0755 and app-user-traversable even under a root umask of 077, which is the consistent version of the partial .staging chmod. No secret-leak concern: release dirs hold only non-secret content (wheel/lock/non-secret config/venv/dataset/docroot); emit-config still chmod 0640s its own output explicitly, and mktemp -d stays 0700 regardless of umask. ✅ codex P2 (reuse read-only) — good catch & fix. The old rm -rf $RELEASE_DIR/dataset + re-extract ran before the stage-only exit, so a same-refs rehearsal against the currently-active release could have yanked the live dataset out from under running consumers. Diff-verifying a scratch extract against the on-disk dataset (fail-closed on mismatch, release dir never written) restores the "stage-only mutates nothing" contract. Combined with the manifest tar-digest check, on-disk == tar == manifest transitively. ◐ F1 (consumer set) — highest-value piece taken, rest appropriately deferred. kayak-healthcheck.timer (the frequent live DB reader) is now in the default KAYAK_UNITS, and deploy.env.example warns that the list gates the cutover check. Confirmed still-outstanding on this host and explicitly deferred to the 4C runbook: kayak-heartbeat (daily, reads ~/DB/kayak.db) and kayak-recap/kayak-config-drift (run engine code from the old /home/pat/kayak checkout). Those are read-only / non-DB, so no corruption risk in the deploy window — but recap running old code post-cutover is exactly the gate's stated concern, so the 4C runbook must enumerate the complete set (the comment commits to deriving it from installed kayak-* timers there). Fine to defer with that written commitment. Notes I'm satisfied are covered, not gaps: SITE_URL flowing from the app-env allowlist → run_app/runuser → the activation emit-config rides the deployer's exported env, the same propagation the activation tests already exercise via KAYAK_HOST_ENV; the unit test covers the data-read up through the staged config. F4 (host-config fingerprint inert — no /etc/kayak/host.yaml), F5 (HEALTH_URL required for first cutover), and F6 (schedule clear of the :12 pipeline) remain runbook items, as noted in the commit. Bottom line: the real finding (root source-injection) is closed and I've verified it against this host's actual .env; the rest are correct. No blockers — mergeStateStatus is CLEAN. The remaining consumer-set enumeration is the one thing the 4C cutover runbook must not skip. Re-reviewed against the live WKCC host, 2026-06-13. 🤖 Generated with Claude Code |
Sorry, something went wrong.
…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 is what actually retires #228. `[build-system].requires` was unbounded, requirements-build.lock is `uv pip compile`d from it, and CI's drift check recompiles against live PyPI every run and diffs — so hatchling 1.31.0 shipping reddened every open PR, whatever its diff. #228 regenerated the lock by hand; that bought time, it didn't fix anything. A range does not fix it either, which is worth stating because it is the obvious reach: `uv pip compile` resolves the NEWEST version a specifier admits, so `>=1.27,<2` re-resolves the day 1.32.0 lands and the lock is stale again. Measured, not assumed: hatchling>=1.27,<2 -> 1.31.0 (newest in range — drifts again) hatchling>=1.27,<1.31 -> 1.30.1 (proves it takes the max, not a pin) hatchling==1.30.1 -> 1.30.1 (stable) Only `==` makes compile-then-diff idempotent, which is the property the check needs. It also fits what this feeds: kayak-deploy builds the wheel with --no-build-isolation from the hashed lock (#190), so the backend version is part of the artifact and pinning it is the point, not a wart. The cost is that nothing watches this line — no Dependabot ecosystem reads [build-system].requires, so hatchling now goes stale until bumped by hand. A stale-but-working build backend is a strictly better failure than an unpredictable red CI, and the comment carries the two-line bump recipe. Lock unchanged: #228 already put 1.31.0 there, so this only removes the mechanism that made that a recurring chore. Drift check verified idempotent across repeated runs; wheel builds clean. Claude-Session: https://claude.ai/code/session_01RR6LwVdry9fmXkEhkC9Cbi Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Second slice of finishing-plan Batch 4: the paired-release activation orchestrator (deploy/kayak-deploy.sh, installed as /usr/local/sbin/kayak-deploy by the 4C runbook). Nothing on the current host consumes it yet — fully inert until the 4C cutover re-points nginx and installs it.
Design (plan §S7, trimmed per accepted decision D2)
Tests
Ref-shape rejection, missing-conf failure, unreachable-SHA rejection (fast), plus a slow end-to-end staging run using this repo's HEAD as the engine and the fixture dataset as a local git repo — asserts the venv, snapshot, runtime config, digest manifest, and that stage-only never creates current. bash-3.2-compatible (suite runs on the dev Mac); prod needs only python3+pip3+git.
The activation phase gets its first real exercise in the Batch 4C clean-VM rehearsal (the planned virgin install), before the prod cutover.
Gate: ruff + format, pytest (1718 + the slow staging test), mypy, shellcheck — green.
🤖 Generated with Claude Code