Skip to content

Test the backup and 404 flows, put the pull in git, and describe every config value once - #62

Merged
ptr727 merged 14 commits into
developfrom
record-production-deploy
Aug 8, 2026
Merged

Test the backup and 404 flows, put the pull in git, and describe every config value once#62
ptr727 merged 14 commits into
developfrom
record-production-deploy

Conversation

@ptr727

@ptr727 ptr727 commented Aug 8, 2026

Copy link
Copy Markdown
Owner

The maintainer asked for the two mechanisms under Recurring operations to be tested rather than read. Both were. What they found turned into the rest of this branch: the pull script had nowhere to live, its host values had to be retyped by hand, and every configuration value was described in whichever file its author happened to be editing.

The two mechanisms, exercised

The backup, without spending the evidence. systemctl start would have written the second journal entry that 2026-08-09 exists to prove, so the transport was exercised with --dry-run and the schedule was read instead. The journal still holds exactly one entry and the timer's LAST is still -, so tomorrow is genuinely the first time it fires.

The 404 review, end to end. 9,996 edge requests, 9,285 of them this repo's own deploy gate. Of the 711 that remain, 101 were site-host 404s across 73 distinct paths, and every one was a scanner shape. Only / and /robots.txt touch the URL contract and both are explained. Nothing is owed to golden-urls.txt, recorded as a clean pass because an unrecorded one is indistinguishable from a pass nobody ran.

Four things the run found, each written where it binds:

  • A referer does not implicate this site unless it points somewhere else. All 36 referer-bearing site-host 404s were scanners setting Referer to the request URL, so the rule as written reported three dozen broken links on a site that had none.
  • ServiceName separates an edge 404 from a site 404 inside one log, which the tier table described conceptually and gave no way to apply. 99 of 101 reached the site. The two that did not are the VPS's tier and have been raised with them.
  • jq 'select(...)' | wc -l counts lines, not records. It reported 37 and 1,332 where the truth was 1 and 36.
  • --dry-run named no files, so the one flag that answers "what will tomorrow bring" could not answer it.

ops/, because the pull had no copy anywhere

The script that makes the off-host copy lived only on the backup host, in /usr/local/bin and an untracked directory beside it — the same machine the copies live on. It is now committed with both units, an installer, and a README.

Nothing in ops/ names a machine. VPS_SSH_HOST, BACKUP_ARCHIVE_ROOT and LOG_ARCHIVE_ROOT have no defaults and preflight refuses to run without them, since a wrong-but-valid destination is a backup nobody can find. User= and RequiresMountsFor= live in a drop-in, and the script refuses to run as root rather than warning about it.

ops/install.sh derives every host value from secrets/<server>.<environment>.env rather than asking for it. Two derivations where the obvious answer is wrong, both caught by running it:

  • The group comes from the destination, not id -gn. Here those differ, and Group= sets the process's primary group, so the obvious answer creates files the existing tree's group cannot read — and only where no setgid bit masks it, so it half works.
  • RequiresMountsFor= needs the mount point, not the destination below it.

Installed and verified: byte-identical to the committed copy, systemd resolving User, Group and RequiresMountsFor from the drop-in, environment file 600 root:root, journal still one entry.

ENVIRONMENT.md, and a gate so it stays true

Every configuration value is described once, grouped by where it lives, because that determines who can change it and what happens when it is wrong. The .example files stated the format and the scripts state the defaults; both now point here.

The two templates collapse into one example.env. .gitignore needed a negation for it*.env matched the template, which is what the old .example suffixes had been dodging. Anchored, so a real environment file anywhere is still ignored.

checks/check-env-docs.py runs in CI and fails both directions: a value declared with no row, and a row describing something declared nowhere, which is the shape a rename leaves. Tested by breaking it each way rather than trusting a clean first run.

Corrections to the record

  • The pull's two sides had grown four duplicate names for the same directories, needing a mapping table here and a translation step in the installer. Unified, and both are deleted rather than maintained.
  • The script's own install instructions pointed at /usr/local/sbin/, which nothing runs. scp and chmod would both report success while the timer kept running the previous copy.
  • The archive directory carries MANIFEST.txt as well as RECONCILE.md, so the file count exceeds the number of logs by two rather than the one this PR's first commit recorded.
  • SSH_OPTS was unquoted and contains spaces, harmless while only systemd read it and broken once a shell sources the same file. The comment claiming systemd cannot strip quotes was wrong, measured with systemd-run rather than assumed.
  • ops/ was not in the shell-lint gate the repo's other two scripts pass. Both are now, formatted and with two shellcheck findings suppressed with reasoning at the site.

Verification

The mirror gate does not apply: no change to deploy/Caddyfile, deploy/maps/, content/, static/, hugo.yaml or layouts/, which OPERATIONS.md exempts explicitly. Every other CI step was run locally through the Docker images GOVERNANCE.md names.

Gate Result
markdownlint 18 files, 0 issues
cspell 0
actionlint 0
editorconfig-checker 0
shellcheck + shfmt 4 scripts clean
config validation OK
check-env-docs.py 28 values, all described
hugo --panicOnWarning clean
check-url-parity.py PASS, 328 render, 778 media, 1049 assets

The hub's prose_lint.py is at baseline on every file: nothing I authored adds a finding.

The State table and the robots.txt entry both said production had not
been redeployed. It has: release 20260808-154717, from main, by
pipeline, with the 1,245-URL contract verified against the live site.
Read from the served bytes rather than the run's exit code: /robots.txt
answers 200 advertising the .net sitemap, /robots.txt/ 301s to it,
sitemap.xml is 312 .net and zero .com, and the gallery fix is live.

Three things learned since about the off-host copy, all recorded where
the log review will read them rather than left in a channel file.

The stale misnamed archive is deleted and the count reads 58 again. The
general case has a mechanism now: the host keeps an append-only
RECONCILE.md inside the archive directory, so the pull carries it with
the data it explains instead of depending on a note nobody rereads.

A copy predates the timer. Directory mtimes on the backup host are copy
times where file mtimes are the source's, and they show a pull at 03:31
UTC that the service journal never recorded, because the script was run
directly rather than through systemd. A journal with one entry is not
evidence of one copy.

Nothing else had diverged, audited both ways: logs identical, 38
archives shared and identical in size, four newer on the VPS because
they postdate the pull, four older retained off-host because the pull
passes no --delete.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 8, 2026 16:00

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the repository’s operational documentation to reflect the latest production deploy state and to move/record lessons learned from the off-host log audit in a durable location for future log reviews.

Changes:

  • Update TODO.md State/operations notes to reflect the redeploy, /robots.txt behavior, and off-host audit findings/lessons.
  • Update OPERATIONS.md “Log Review” guidance to record reconciliation handling (RECONCILE.md) and clarify why service journals alone can’t prove copy history.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
TODO.md Refreshes migration status records (production release, robots verification) and captures off-host audit lessons in the running backlog.
OPERATIONS.md Extends log-review operational guidance with rename reconciliation mechanics and copy-timestamp interpretation.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread TODO.md Outdated
ptr727 and others added 9 commits August 8, 2026 09:25
… like

Both halves were exercised without running the service, so the second
journal entry stays the proof that 2026-08-09 09:00 UTC fired the timer
rather than a hand. The transport was checked with --dry-run and the
timer was read: NEXT 09:08:45 UTC, LAST still `-`.

Four things the run found, each recorded where it binds rather than
only in a session log:

- A referer does not implicate this site unless it points somewhere
  else. All 36 referer-bearing site-host 404s were scanners setting
  Referer to the request URL, so the unrefined rule reported three
  dozen broken links on a site that had none.
- ServiceName separates an edge 404 from a site 404 inside one log,
  which the tier table described conceptually and gave no way to
  apply. 99 of 101 reached the site; 2 did not.
- jq 'select(...)' with no projection pretty-prints, so piping it to
  wc -l counts lines rather than records. It reported 37 and 1,332
  where the true counts were 1 and 36.
- The archive directory carries MANIFEST.txt as well as RECONCILE.md,
  so the file count exceeds the number of logs by two, not one.

The outward pass itself was clean: 73 distinct site-host 404 paths,
every one a scanner shape, nothing owed to golden-urls.txt.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… the log leg

The header's own deploy command wrote /usr/local/sbin/vps-backup-pull.
The service runs /usr/local/bin/vps-backup-pull and sbin/ is empty, so
following it installs a copy nothing executes while scp and chmod both
report success and the timer keeps running the previous one. The VPS
canonical had already corrected this and explains why bin/ is deliberate:
the script authenticates with the pieter key and must not run under sudo.

Measuring that turned up the larger problem. The two copies have diverged
in both directions, not one. This side has the entire access-log leg,
the tell() fix and the VERIFIED counter; the canonical has the corrected
install block and the no-sudo rationale. An scp from the canonical
therefore deletes the log pull outright, which is the half the log review
runs on -- where the record said it would cost only the --quiet fix.

The stale block is replaced in the patch copy with the canonical's
wording, plus a refusal to run that scp until #53 reconciles the two.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every VPS fact was recoverable only by asking: the host and the log
paths lived in the pull script's own defaults and a gitignored session
note, "The Channel" used a <vps-host> placeholder with no value behind
it, and Log Review told you to read an archive without saying where it
is. So the docs described the work and could not start it.

Six values now name what a machine owns rather than what the project
does, following the CAPTURE_ROOT precedent -- environment-independent,
so they sit in the default file:

  VPS_SSH_HOST  VPS_TRAEFIK_LOG  VPS_TRAEFIK_LOG_ARCHIVE
  VPS_COMMS_DIR  LOG_ARCHIVE_ROOT  BACKUP_ARCHIVE_ROOT

All six verified against both ends before being written down.

A new "Working With the VPS" section is the entry point: the values,
the two credentials and why confusing them fails badly, why the copy
is a pull, why today's traffic is never in it, and pointers to the
four sections that already cover the rest.

The channel transfers are documented as the one deliberate exception.
An allow rule matches command text rather than its expansion, so
substituting the variables there converts an allowlisted transfer into
one that prompts while looking like a tidy-up.

Also records the measured backup exposure: duplicacy mounts /data only,
so the canonical is protected three ways while the copy that actually
runs -- the superset carrying the log leg -- is in no snapshot, no
archive and no repository.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The script that makes the off-host copy was the one thing with no copy
anywhere: /usr/local/bin on the backup host and an untracked directory
beside it, on the same machine the copy lives on. It is now ops/, with
both units, an EnvironmentFile template, a drop-in template, and a
README covering what it does and how to check it.

Nothing in ops/ names a machine. VPS_HOST, DEST and LOG_DEST have no
defaults and preflight refuses to run without them, because a
wrong-but-valid destination is a backup nobody can find. User= and
RequiresMountsFor= move to a drop-in for the same reason, and the
script now refuses to run as root rather than warning about it, since
that is what a missing drop-in would otherwise produce.

The guard sits ahead of the banner deliberately. A banner printed with
empty values reads, in a journal skim, exactly like a run that started
and then failed somewhere real.

OPERATIONS.md "Working With the VPS" names the script, and states which
of its variables pair with this repo's own: DEST is BACKUP_ARCHIVE_ROOT
and LOG_DEST is LOG_ARCHIVE_ROOT. Neither side can source the other's
file, so the correspondence is stated rather than enforced.

Gates run per GOVERNANCE.md, through Docker rather than npx: markdownlint,
cspell, editorconfig-checker, and the hub's prose_lint.py. The last one
found British spelling and an observed home path in prose I had written,
and both files are back to their baseline counts.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit left three values to be set by hand in /etc, having
just recorded the same three in secrets/local.production.env under the
names the log review uses. That is the drift OPERATIONS.md warns about,
created by the change that warned about it, and it made installing the
maintainer's problem for no reason.

ops/install.sh derives all of it: VPS_SSH_HOST, BACKUP_ARCHIVE_ROOT and
LOG_ARCHIVE_ROOT map to VPS_HOST, DEST and LOG_DEST; the account is
whoever runs it; the group is read from the destination; the mount comes
from findmnt. Generating one vocabulary from the other is also what stops
the pair from disagreeing later.

Two derivations where the obvious answer is wrong, both caught by running
it rather than by reading it:

- The group must come from the destination, not `id -gn`. Here those are
  users and pieter, and Group= sets the process's primary group, so the
  obvious answer creates files the existing tree's group cannot read --
  and only where no setgid bit already masks it, so it half works.
- RequiresMountsFor= needs the mount point, not the destination below it.

--check needs no root, prints both generated files, and verifies SSH. It
reports "needs root to compare" rather than "already correct" when it
cannot read an existing file, since an installer claiming agreement it
never checked is the failure this is written against.

Verified: --check output matches the running configuration exactly, and
the real run stops at the first sudo having written nothing, leaving the
working script installed and tomorrow's timer armed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…apart

The pull and the secrets file had grown separate names for the same four
directories: VPS_HOST/VPS_SSH_HOST, DEST/BACKUP_ARCHIVE_ROOT,
LOG_DEST/LOG_ARCHIVE_ROOT, and REMOTE_TRAEFIK_LOG_DIR against
VPS_TRAEFIK_LOG_ARCHIVE. I introduced the second set rather than reusing
the first, on the reasoning that DEST is too generic to sit beside
DEPLOY_ROOT in a shared file. That bought a bare-name collision hazard
and paid for it with a duplicate vocabulary.

The cost compounded rather than staying put. Two names needed a pairing
table in OPERATIONS.md to say which matched which, a paragraph on why
neither side could source the other's file, and a translation step in
the installer. All three were machinery for a problem that did not have
to exist.

The pull now uses the descriptive names throughout: VPS_* is something
on the VPS, *_ROOT is something on the backup host. The table, the
paragraph, and the mapping are deleted rather than maintained, and
install.sh copies values instead of translating them.

Verified: the pull runs directly off secrets/local.production.env with
no translation at all, which is the property that was missing before;
it still fails closed with the new names; install.sh --check derives
every value correctly; and no old name survives anywhere in the tree.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The descriptions were spread across two .example files, three documents
and a script header, so a value could be described twice and disagree
with itself, and a new one could be added with no description at all.

ENVIRONMENT.md is now the single description of all 28 values, grouped
by where they live, because that determines who can change one, what
happens when it is wrong, and whether it reaches a public history. The
.example files stated the format and the scripts state the defaults,
and both now point here rather than restating.

The two templates collapse into one example.env at the root, covering
both destinations in marked sections. `.gitignore` needed a negation
for it: `*.env` matched the template, which is what the old
`.example` suffixes had been dodging. Anchored, so it exempts only the
root file and a real environment file anywhere is still ignored.

checks/check-env-docs.py runs in CI and fails both directions: a value
declared in the template, in a workflow's vars/secrets, or listed as a
per-invocation knob and having no row, and a row describing something
declared nowhere, which is the shape a rename leaves behind. Tested by
breaking it each way rather than by trusting a clean first run.

Two bugs found while writing it:

- SSH_OPTS was unquoted and contains spaces. Harmless while only
  systemd read it; example.env is also shell-sourced, where it ran
  everything after the first space as a command.
- The comment claiming systemd cannot strip quotes was wrong. Measured
  with systemd-run: it strips them. Quoting is correct for both readers.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two shell scripts were added to a repo that shellcheck-and-shfmt gates
its shell, and neither was in the list, so CI would have said nothing
about them indefinitely.

Both now pass the gate unchanged in behavior:

- shfmt -w reformatted them. install.sh takes tabs from .editorconfig
  [*.sh]; vps-backup-pull carries no extension so it takes the [*]
  default of four spaces. Formatting only, proven by diffing the dry-run
  output of the old and new scripts rather than by reading the diff:
  identical except rsync's own byte counters, which vary per run.
- Two shellcheck findings are suppressed with the reasoning at the site.
  SC2329 on cleanup(), which the EXIT trap invokes and shellcheck cannot
  see. SC2029 on the checksum ssh, where the local expansion of
  VPS_ARCHIVE_DIR is the intent and the *.enc glob stays quoted for the
  remote shell.

Running shfmt -w through Docker writes as root and takes ownership of
the files it touches. The workflow comment now says to use -d, which is
what CI runs and what avoids it.

The installed copy predates the reformat, so it is no longer byte-
identical to the committed one. Normalizing it through shfmt and diffing
shows the only content difference is the two directives, so the running
backup is correct, and re-running ops/install.sh restores the identity.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The reinstall after the shell-gate reformat restores byte-identity
between /usr/local/bin/vps-backup-pull and ops/vps-backup-pull, so the
running backup is verifiably the reviewed one rather than merely
equivalent to it.

It also exercised the installer's idempotent path for the first time:
both generated config files reported already correct, only the script
was replaced, and the summary took the unchanged branch. So applying a
changed value means running the installer again rather than editing
anything on the host.

The timer's LAST is still `-` and the journal still holds one entry, so
2026-08-09 remains the first scheduled run, which is the whole point of
never having started the service by hand.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 8, 2026 18:00
@ptr727 ptr727 changed the title Record the production deploy, and what the off-host audit found Test the backup and 404 flows, put the pull in git, and describe every config value once Aug 8, 2026
The State table said production serves 20260808-154717 and a bullet
twelve lines below said 20260808-041050. The header on the live site
says 154717, so the bullet was describing the first production deploy
while reading as current.

Rewritten to state what is served now, with the 9/9 host-side
verification kept and attributed to the release it actually covered.

Found by Copilot on PR #62.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated no new comments.

Suppressed comments (3)

ops/install.sh:111

  • The generated /etc/vps-backup-pull.env sets SSH_OPTS without quotes, but this value contains spaces. That breaks if the env file is ever sourced by a shell (it will try to execute "ConnectTimeout=15" as a command) and may also be parsed unexpectedly by systemd. Quote the value to match example.env and to keep the generated env file safe to reuse.
BACKUP_ARCHIVE_ROOT=$BACKUP_ARCHIVE_ROOT
LOG_ARCHIVE_ROOT=$LOG_ARCHIVE_ROOT
SSH_OPTS=-o ConnectTimeout=15 -o BatchMode=yes
EOF

ops/vps-backup-pull:25

  • This comment references CONFIG.md, but there is no CONFIG.md in the repository. That makes the header misleading and sends readers to a dead document; reference the actual doc that explains the pull-vs-push security rationale (e.g., OPERATIONS.md "Working With the VPS").
# Why a PULL and not a push from the VPS: it keeps the VPS free of any credential
# reaching another system, which is the one security rule in CONFIG.md item 0.
# This host already has an SSH key trusted by the VPS; the VPS has none for here.

ops/vps-backup-pull:296

  • In the --dry-run early-exit path, the completion summary is printed via say(), so it is suppressed under --quiet. That contradicts the script's "--quiet = errors and summary only" behavior and the tell() change that was added specifically to keep summaries visible under --quiet.
say
if [[ $DRYRUN -eq 1 ]]; then
    say "=== dry run complete -- nothing changed"
    exit 0

Copilot AI review requested due to automatic review settings August 8, 2026 18:03

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated no new comments.

Suppressed comments (1)

ops/vps-backup-pull:303

  • Because set -euo pipefail is enabled, the du | cut pipelines will terminate the script if du fails, but the 2>/dev/null redirection hides the underlying error. That can turn an otherwise-successful pull into a non-obvious failure right at the summary step. Consider letting du emit its error and failing with an explicit message so the journal shows why the run exited non-zero.
ARCHIVES=$(find "$BACKUP_ARCHIVE_ROOT/pangolin" -name '*.enc' -type f | wc -l)
SIZE=$(du -sh "$BACKUP_ARCHIVE_ROOT" 2>/dev/null | cut -f1)
if [[ $LOGS -eq 1 ]]; then
    LOGSIZE=$(du -sh "$LOG_ARCHIVE_ROOT" 2>/dev/null | cut -f1)
    tell "=== done -- $ARCHIVES archive(s), $VERIFIED verified, $SIZE in $BACKUP_ARCHIVE_ROOT; $LOGFILES log file(s), $LOGSIZE in $LOG_ARCHIVE_ROOT"

Suppressed findings carry the real defects often enough that reading
them is the rule here. All four reproduced.

- A failing du aborted the run silently. `set -e` with `pipefail` makes
  the assignment fatal and 2>/dev/null hid the cause, so a pull that
  copied everything exited 1 at the summary with an empty journal.
  Reproduced against a missing path: no output, exit 1. The size is
  cosmetic, so du now reports to stderr and the run carries on with the
  transfer already done.
- The generated /etc/vps-backup-pull.env still wrote SSH_OPTS unquoted,
  the same defect fixed in example.env one commit earlier and missed in
  the file that generates it. Harmless while only systemd reads it, and
  broken the moment a shell sources the same content.
- The dry-run early exit printed its summary through say(), so
  `--quiet --dry-run` printed nothing at all, contradicting the flag's
  own promise of errors and the summary. It uses tell() now, which is
  what the summary path already did.
- The header cited CONFIG.md as though it were in this repository. It is
  /root/CONFIG.md on the VPS, confirmed present there, so the reference
  now says which host holds it and points at the local equivalent.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 8, 2026 18:10
@ptr727

ptr727 commented Aug 8, 2026

Copy link
Copy Markdown
Owner Author

Answering the four suppressed findings, which have no threads to resolve. All four were real and all four are fixed in 5e3ef21. Each was reproduced before being accepted.


ops/vps-backup-pull:303 — Because set -euo pipefail is enabled, the du | cut pipelines will terminate the script if du fails, but the stderr redirection hides the underlying error.

Correct, and it is the worst-shaped of the four. Reproduced against a missing path: the script printed before, then exited 1, and the line after the assignment never ran.

So a pull that copied every archive and every log would exit 1 at the summary, having done all its work, and leave a journal with no indication of why. Under the timer that reads as a failed backup.

The size is cosmetic, so the fix keeps the transfer's success rather than the size's: du now reports to stderr where the journal can see it, and the run continues with size unavailable.


ops/install.sh:111 — The generated /etc/vps-backup-pull.env sets SSH_OPTS without quotes, but this value contains spaces.

Correct, and worse than it looks: the same defect was fixed in example.env one commit earlier and missed in the file that generates the real one.

Quoting is safe for both readers, measured rather than assumed. A systemd-run unit with an EnvironmentFile containing a quoted value received it with the quotes stripped, identical to the bare form. So systemd is indifferent, and the quoted form is the only one that also survives a shell sourcing the same content.

Note for anyone re-running the installer: /etc/vps-backup-pull.env on the backup host still holds the unquoted form, so ops/install.sh will now report it as differing and stop. That is the intended behavior. Re-run with --force to replace it.


ops/vps-backup-pull:296 — In the --dry-run early-exit path, the completion summary is printed via say(), so it is suppressed under --quiet.

Correct. --quiet --dry-run printed no summary at all, which contradicts the flag's own documented contract of errors and the summary. It is the same defect tell() was introduced to fix on the normal summary path, and the dry-run exit was missed. It uses tell() now.


ops/vps-backup-pull:25 — This comment references CONFIG.md, but there is no CONFIG.md in the repository.

Correct that it is not in this repository, and the reference was misleading. The document is real but lives on the other host, as /root/CONFIG.md, confirmed present there. It is one of the paths the VPS advertises for the host-config leg, so the pull copies it.

The comment now names the host that holds it and points at OPERATIONS.md "Working With the VPS" for the same reasoning stated locally.


Coverage

The review reports 15 of 16 changed files and names no file list, so which file went unread cannot be determined from the API. Flagging rather than re-requesting: on record here a partial has never cleared on a re-request, and it is the maintainer's call.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated no new comments.

Suppressed comments (5)

ops/vps-backup-pull:22

  • This comment implies the script itself reads /etc/vps-backup-pull.env. In reality it only reads environment variables; systemd loads /etc/vps-backup-pull.env via EnvironmentFile, but a direct shell invocation won’t pick it up unless the caller sources/exports it. Tightening this wording avoids confusion when running the script manually.
# Paths below are read from /etc/vps-backup-pull.env, so a host states its own layout
# there rather than editing this file. See example.env and ENVIRONMENT.md.

ops/vps-backup-pull:16

  • The script header says to schedule this via a user crontab/user systemd timer ("not root's"), but this PR also provides a system-level systemd timer/service under /etc/systemd/system. That guidance is misleading; the important constraint is that the unit runs as a non-root user via User= (drop-in), regardless of whether it’s a system timer or user timer.

This issue also appears on line 21 of the same file.

# the unit sets User=. Same applies when scheduling it: use the user's crontab or a
# user systemd timer, not root's.

ops/install.sh:55

  • ENV_FILE is taken verbatim, so a relative ENV_FILE will resolve against the caller’s current working directory (unlike deploy/make-release.sh, which normalizes relative ENV_FILE against the repo and refuses traversal). That makes it easy to accidentally read the wrong environment file during install/check.
REPO=$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)
ENV_FILE=${ENV_FILE:-$REPO/secrets/local.production.env}

[[ $EUID -ne 0 ]] || die "do not run this under sudo -- run it as the account that will own the backup; it calls sudo for the steps that need it"
[[ -f $ENV_FILE ]] || die "$ENV_FILE does not exist (ENV_FILE overrides which file is read)"

checks/check-env-docs.py:6

  • The docstring says this check fails when a value is “declared or consumed anywhere”, but the implementation only treats templates (example.env), workflow refs, and the explicit KNOBS list as the declared surface. Narrowing the wording avoids over-promising coverage.
"""Fail if a configuration value is declared or consumed anywhere but not described in
ENVIRONMENT.md, or described there and declared nowhere.

ENVIRONMENT.md is the single description of every configuration value. A new value gets
added wherever its author is working, which is rarely the doc, and no linter notices a

ops/README.md:51

  • install.sh currently always rewrites /etc/vps-backup-pull.env and the drop-in (via sudo install), even when they already match. This sentence claims matching files are “left alone”, which isn’t accurate (even if the content is identical).
Re-running is safe and is how a changed value is applied. A file that already matches is reported and left alone, and one that differs stops the run until `--force`.

Second round of low-confidence findings, and the ratio held: every one
reproduced.

- ops/install.sh took ENV_FILE verbatim, so a relative name resolved
  against the caller's directory. deploy/make-release.sh in this same
  repo normalizes against the repo and refuses traversal, and the
  installer is the one that writes to /etc, so it now applies the same
  rule. Verified from /tmp and against a traversing name.
- The header said paths are read from /etc/vps-backup-pull.env, which
  the script never opens. systemd loads it through EnvironmentFile, so
  a direct run picks up nothing unless the caller sources it first.
- The header told the reader to schedule via a user timer rather than
  root's, while this PR ships a system timer. The account is what
  matters and the drop-in sets it, so the guidance now says that.
- check-env-docs.py claimed to cover a value "declared or consumed
  anywhere". It reads example.env, workflow vars/secrets refs, and the
  KNOBS list. Widening it is not the fix, because a script-local name
  and a configuration value are the same shape, so the docstring now
  states the boundary and why it is there.
- ops/README.md said a matching file is left alone. Both generated files
  are rewritten every run; the comparison is a report and the --force
  guard, not a skip.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 8, 2026 18:19
@ptr727

ptr727 commented Aug 8, 2026

Copy link
Copy Markdown
Owner Author

Second round of suppressed findings, again with no threads to resolve. All five were real and all five are fixed in e7e6b15. That is nine for nine across both rounds, which is worth saying plainly given they arrive marked low confidence.


ops/install.sh:55ENV_FILE is taken verbatim, so a relative ENV_FILE will resolve against the caller's current working directory (unlike deploy/make-release.sh, which normalizes relative ENV_FILE against the repo and refuses traversal).

Correct, and the comparison to make-release.sh is exactly right — that script normalizes against the repo and rejects a traversing name, and the installer did neither. It is also the worse place to have the gap, because this is the script that writes to /etc and enables a timer.

It now applies the identical rule. Verified both ways: run from /tmp with a relative ENV_FILE, it resolves against the repo and derives the same values; given a traversing name it refuses with ENV_FILE must not traverse.


ops/vps-backup-pull:22 — This comment implies the script itself reads /etc/vps-backup-pull.env. In reality it only reads environment variables.

Correct. The script opens no file. systemd loads it through the unit's EnvironmentFile, so a direct run picks up nothing unless the caller sources an environment file first — which is exactly what I had to do to exercise it by hand, without noticing the comment said otherwise. Reworded to say the script reads the environment, name what loads the file under the timer, and state that a manual run has to source one.


ops/vps-backup-pull:16 — The script header says to schedule this via a user crontab/user systemd timer ("not root's"), but this PR also provides a system-level systemd timer/service under /etc/systemd/system.

Correct, and the guidance predates what this PR ships. It came across from the copy on the other host, where no unit existed. The constraint that matters is the account, which the drop-in sets with User=, and a system timer satisfies it as well as a user timer does. Reworded to say that rather than to rule out the thing being shipped.


checks/check-env-docs.py:6 — The docstring says this check fails when a value is "declared or consumed anywhere", but the implementation only treats templates, workflow refs, and the explicit KNOBS list as the declared surface.

Correct, and over-promising in a docstring is worse than usual here, because the whole point of the check is that nobody should have to remember what it covers.

Widening the implementation is not the fix, though, and the docstring now says why: a script-local variable and a configuration value are the same shape, so scanning for reads would report the difference as findings nobody can clear. The boundary is now stated explicitly, including that a new knob has to be added to KNOBS by hand.


ops/README.md:51install.sh currently always rewrites /etc/vps-backup-pull.env and the drop-in (via sudo install), even when they already match. This sentence claims matching files are "left alone", which isn't accurate.

Correct. Both generated files are written unconditionally in the install path. The comparison is a report and the --force guard, not a skip, and the README now says that instead of describing behavior the code does not have.


Coverage

Still 15 of 16 files with no file list, so the unread file cannot be identified from the API. Reporting rather than re-requesting, per the same reasoning as the previous round.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.

Comment thread ops/vps-backup-pull
Both roots are chmod 700'd and written into, so a wrong value is damage
to the host rather than a failed backup. `/` is the case that matters:
`chmod 700 /` locks every other account out, and nothing downstream
would have refused it. The preflight only checked that the value was
non-empty.

Now checked: absolute, not `/`, and no trailing slash. A relative value
is refused for the reason the deploy tooling refuses one, since it means
a different directory depending on where the caller stood, and under the
timer that is systemd's working directory. LOG_ARCHIVE_ROOT is checked
only when the log leg runs, so --no-logs is unaffected.

Also fixes --check, which died on the first differing file. It is
documented as validate-and-print-touching-nothing, so stopping there hid
the second file and the reachability test behind it. The --force
requirement belongs to the install path and now applies only there.

Raised by Copilot on PR #62.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 8, 2026 18:25

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated no new comments.

@ptr727
ptr727 merged commit a07ae2f into develop Aug 8, 2026
5 checks passed
@ptr727
ptr727 deleted the record-production-deploy branch August 9, 2026 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants