Repository navigation
Rocky deployment, archives, and operational tooling - #5
Conversation
52797db to
3a25a98
Compare
af42bf0 to
8f2783c
Compare
|
Independent adversarial review resolutions (latest head
|
3a25a98 to
8fcd548
Compare
8f2783c to
50d7beb
Compare
|
Deployment requirement correction in
|
495fd04 to
b8b7e2d
Compare
ahernsean
left a comment
There was a problem hiding this comment.
Review — Phase 4 deployment, archives, and operational tooling
Archive replication with independent re-verification, the quarantine-on-restore flow, and the signed restore request are all sound ideas. Several of them don't survive contact with a host that isn't yours, and one retention bug silently deletes archives you meant to keep.
1. Retention's pre-migration guard is dead code, and pre-migration archives evict daily archives (driving_log/operations.py:149-173)
archives = sorted(archive_dir.glob("driving-log-*.tar.gz"), reverse=True)
keep = set(archives[:14])
...
if archive not in keep and ".pre-migration." not in archive.name:Two problems, both from the actual filename produced by db.py:82, which is driving-log-pre-migration-v1-20260101T000000000000Z.tar.gz:
- The guard tests for
.pre-migration.with dots. The real name uses hyphens, so the guard never matches anything. (Those files happen to survive anyway, via the unparseable-stamp branch at:161-163— but by accident, not by the check that was written for it.) - Sorting is lexicographic on the filename, and
driving-log-p…sorts above everydriving-log-2026…. So each pre-migration archive occupies one of the 14 "most recent daily" slots.
Demonstrated with 20 daily archives + 3 pre-migration archives:
daily kept: 13 of 20 # 11 by the daily rule instead of 14, plus weekly-rule survivors
removed: 20260302..20260309 (7 recent dailies deleted)
test_retention_keeps_fourteen_daily_and_eight_weekly only creates timestamp-named files, so neither behaviour is covered. Sorting by parsed timestamp (or by st_mtime, as the rest of the codebase does) and excluding pre-migration archives before slicing would fix both.
This matters right now because #6 introduces schema v2 — the first migration that will actually produce these files on the Rocky host.
2. doctor crashes on any host without Tailscale or systemd-user (driving_log/operations.py:97-126)
subprocess.run(..., check=False) handles a non-zero exit; it does not handle a missing binary. FileNotFoundError propagates out of service_snapshot/tailscale_snapshot, and both are called from cli.doctor outside the try block that guards the database section. Running the suite here:
FAILED tests/test_foundation.py::ApplicationTests::test_cli_doctor_json
FAILED tests/test_foundation.py::ApplicationTests::test_health_endpoints_and_doctor
E FileNotFoundError: [Errno 2] No such file or directory: 'tailscale'
2 failed, 60 passed
Both tests call the real doctor() with no isolation, so they pass only on a host with tailscale installed. GitHub's ubuntu-latest doesn't have it, and the rockylinux:9.8 container has neither it nor a systemd user session — which means quality-unit, rocky-sqlite, and security all fail as written. Consistent with that: there are zero check runs on every PR in this stack, so the four-job gate has never actually run. The "62 non-browser tests pass" claim in #6 is host-specific.
Wrapping both snapshots in except (FileNotFoundError, OSError) → {"available": False, ...} makes doctor do its job (report what's wrong) instead of dying, and makes the tests portable.
3. Signed restore requests never expire and can be replayed (driving_log/operations.py:176-207)
The payload carries not_before and nothing else time-related, so a signed request is valid forever. Worse, the run isn't recorded as consumed: result.json is written but never read back, and nothing removes or marks request.json. Re-running
driving-log archive restore-request <operation-id>
# or: systemctl --user start driving-log-restore@<operation-id>.serviceverifies the same signature again, restores the same (now stale) archive again, and pushes the current live database into a fresh quarantine directory. On a household box that's one shell-history arrow-up away from silently reverting the record.
Suggest adding a not_after bound and refusing to proceed when result.json already exists. Also, the HMAC covers only archive_path/archive_sha256/not_before — the operation_id is bound only indirectly, by the parent-directory check at :191.
test_signed_restore_request_restores_and_records_result covers the happy path only; neither replay nor expiry is tested.
4. deploy/README.md opens with a command that doesn't exist
./driving-log bootstrapThere is no bootstrap subparser in cli.build_parser() — the available commands are serve, doctor, db, start|stop|restart|status|logs, install, live, seed, csv, archive, imports. The first step of the deployment runbook exits with an argparse error.
5. The deployment guide serves the record over plaintext HTTP, contradicting the design and this PR's own description
./driving-log install --public-host HOSTNAME:8443 --public-scheme http
tailscale serve --bg --http=8443 http://127.0.0.1:8766The PR body says "exposing this app on Tailscale HTTPS 8443", and DESIGN.md requires "bind only to localhost and expose only through Tailscale Serve over HTTPS." What's documented is --http, i.e. unencrypted inside the tailnet. Combined with the removal of application auth in #4, everything — including the supervisor fields the design calls out as sensitive — crosses the tailnet in the clear, and doctor's production check has been adjusted to accept it (protocol_key = settings.public_scheme.upper()). If plaintext is a deliberate trade-off, it needs to be stated as one in DESIGN.md rather than arriving as a default.
6. Re-running install silently drops unmanaged environment keys (driving_log/operations.py:53-85)
The existing environment file is parsed, but only DRIVING_LOG_OPERATION_SECRET/DRIVING_LOG_FORM_SECRET and DRIVING_LOG_EXTERNAL_ARCHIVE_DIR are carried forward; the file is then rewritten from the fixed dict. Any operator-set DRIVING_LOG_PORT, DRIVING_LOG_DATABASE, DRIVING_LOG_ARCHIVE_DIR, etc. is silently reset on the next install, with os.replace making it non-recoverable.
7. Minor — unit hardening is inconsistent (deploy/systemd/)
driving-log-web.service sets NoNewPrivileges, PrivateTmp, ProtectSystem=strict, and ReadWritePaths. driving-log-archive.service and driving-log-restore@.service — which handle the same data, and in the restore case overwrite the authoritative database — set only UMask=0077. Also, restore@.service's ExecStopPost=systemctl --user start driving-log-web.service starts the web service unconditionally, including when it was stopped deliberately before the restore.
Generated by Claude Code
336fd8a to
d187416
Compare
b8b7e2d to
d29c0e7
Compare
|
Review resolution at
The complete local non-browser gate passes at 95.61% coverage. No merge performed. |
Phase 4 of the supervised driving log stack.
Adds user-systemd web/archive/restore units, daily timer, loopback-only deployment, lifecycle CLI commands, private environment generation, external archive replication and corrected retention, expiring one-use signed restore requests, portable doctor output, and deployment guidance that preserves Wordle port 80 while exposing this app through tailnet-only HTTP on port 8443. Tailscale still encrypts node-to-node transport.
No PR in this stack should be merged during unattended delivery.