Document the local verification loop for content and server changes - #5
Conversation
CI proves the render half of the URL contract and cannot prove the other 917. A redirect is the web server's job, and the validation workflow has no server to point at, so a change to the Caddy config or a generated map is invisible to it. The workflow goes green while the redirect it broke stays broken until someone follows an old link. OPERATIONS.md now states which paths require a local release and live check before a pull request is opened, and why each one does: the Caddyfile because rule order is load-bearing and an over-matching regex is silent, the maps because a regenerated one can lose entries and still parse, content and static because a moved page turns a redirect destination into a 404 the build gate does not follow, and hugo.yaml and layouts because permalink changes move URLs underneath the redirects pointing at them. The loop was run rather than written from memory. Against the current build it reports "PASS - 1245 URLs honored", which is the output quoted in the new section. That also settles a residual delta in the audit, which recorded the redirect half as asserted rather than proven. It is now proven against a running server, though only the local mirror and only by hand. CI still cannot enforce it, and that stays true until staging exists, so the report says so rather than claiming the gap is closed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Not ready to approve
There are a couple of documentation statements in the changed hunks that are currently inaccurate/unclear about which script performs the release step and about “needing an argument,” which could mislead the local verification procedure.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR updates the operational and audit documentation to explicitly describe the required local verification loop for changes that can silently break redirects or URL mappings, clarifying why CI alone can’t validate those behaviors.
Changes:
- Adds a new “Local Verification Before a Pull Request” section to document when to run a local release + live URL check.
- Updates the audit report to reflect that the redirect half is verified locally against a running server (but remains non-automatable in CI without staging).
File summaries
| File | Description |
|---|---|
reports/Blog/audit.md |
Updates the URL-contract audit narrative to reflect local live redirect verification and its limits. |
OPERATIONS.md |
Documents the pre-PR local verification loop and the path categories that require it. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Two corrections from review, both cases of the prose contradicting the commands directly beneath it. The audit said `check-live-urls.sh` releases to the local mirror. It does not. `make-release.sh` installs the release and the check script only verifies against a server that is already running, which is the whole reason the two are separate steps. OPERATIONS.md said neither command needs an argument, while the block below it passed `"$HUGO_BASEURL"` to one of them. `make-release.sh` takes no arguments because it reads `secrets/.env` itself. `check-live-urls.sh` does take a base URL. The block now sources the file first, so the value comes from the environment rather than being typed, which is what the sentence was reaching for. The corrected three-line block was run verbatim in one shell and reports "PASS - 1245 URLs honored". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Not ready to approve
The new OPERATIONS.md table contains an inaccurate hard-coded count (“11 redirect rules”) for deploy/Caddyfile, which should be corrected to keep the documentation factually accurate.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
OPERATIONS.md:40
- The table row claims
deploy/Caddyfilehas “11 redirect rules”, but the currentdeploy/Caddyfilecontains 13redirdirectives (e.g.,redir @post_id ...throughredir @mapped ...). This hard-coded count is inaccurate and likely to drift; consider removing the specific number (or updating it) so the doc stays correct.
| [`deploy/Caddyfile`](./deploy/Caddyfile) | The 11 redirect rules. Rule order is load-bearing, and a regex that matches too much is silent. |
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
The table said the Caddyfile holds "11 redirect rules" and the maps are "5 lookup tables". The file carries 13 `redir` directives, so the count was wrong, and it was incidental anyway: that cell exists to say why the Caddyfile needs a running server, not to enumerate it. Both counts are removed rather than corrected, since a number in prose that has to track a config file drifts the moment either changes. The same imprecision exists in `deploy/README.md`, which claims "11 regex rules plus 5 map files". That file is not in this change, so it is recorded in `TODO.md` for a proper re-derivation instead of being fixed by guess. Raised as a suppressed comment inside the collapsed review body, which carries no thread. Worth noting because a loop that polls only unresolved threads reports a clean pass while a finding like this stands, exactly as the merge gate warns. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Answering the suppressed comment from the review on 6411634, since it carries no thread to reply in. Accepted, the count was wrong. Fixed in b89540b by removing both counts rather than correcting them. That cell exists to say why the Caddyfile needs a running server, not to enumerate it, and a number in prose that has to track a config file drifts the moment either changes. The same imprecision exists in Noting for the record that this finding sat in the collapsed review body with no review thread, so a loop polling only unresolved threads would have reported a clean pass while it stood. That is the case the merge gate's third precondition calls out explicitly. |
There was a problem hiding this comment.
🟢 Ready to approve
The changes are documentation-only, consistent with the repo’s existing CI behavior, and accurately separate local proof from CI enforcement.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
* Record the container appdata path and re-derive the redirect table Two gaps, both surfaced by asking why `secrets/.env` carried only two values. **The appdata path was recorded nowhere.** The container reads three host paths and a release writes only one. `$DEPLOY_ROOT` holds the bundle, while the bootstrap Caddyfile and Caddy's persistent state live under a separate appdata root that no script touches and nothing documented. The bootstrap is outside the bundle deliberately, since rules held there would leave a rolled-back site served by the current release's redirects, but that also means no release refreshes it and a rebuild depended on someone remembering where it goes. `CADDY_APPDATA` now records it in `secrets/.env`, and OPERATIONS.md carries the install command and the mount table. The value stays out of committed files, because this repo names no host path. The deployed bootstrap had drifted from the repo's copy, carrying the pre-rewrite comments and a stale rule count. Functionally identical, the `import` line matched exactly, so nothing was broken. Now byte-identical, and the site was re-verified after the restart at 1245 URLs honored. **The redirect table was wrong in more than its count.** It claimed 11 regex rules where the Caddyfile carries 13 `redir` directives reading 5 map files through 3 `map` blocks. Three destinations were also stale: the date archives, their pagination, and the Blogger monthly archives were all documented as redirecting to `/` when the config sends them to `/all/`. `labels.map` and `terms.map` appeared nowhere. The table is rebuilt against the config and keyed on the Caddy matcher names rather than invented R numbers, so a reader can grep the Caddyfile instead of trusting the doc. Every row was checked: the classes sum to 917 exactly, matching `redirect-urls.txt`, with the 778 legacy image URLs called out as gated separately. `@label` is documented as deliberately outside the contract, since that class is a soft 404 rather than a redirect. Clears the backlog item added when #5 dropped the count rather than guessing at it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Require a restart after any config change, and name the container Review caught a claim that was not just wrong but inverted, and testing it against the running mirror found a second instance that was worse. Caddy expands `import` at config-parse time, for the site config and for the `map` blocks reading `maps/*.map`, and does not watch those files. Only static file requests follow the `current` symlink per request. Proven on the mirror: a map entry present in the live release on disk answered 404 until the container restarted, then 301. That makes the verification loop unsound as it shipped. Change a redirect, release, check without reloading, and the check exercises the previous rules, so a broken redirect reports PASS while the artifact is broken. That is exactly the failure the loop exists to catch, so the restart is now a step in it rather than a footnote. The rollback procedure was the worse instance. It said "no restart and no reload", which would revert the content while leaving the previous release served by the current release's redirects, the precise mismatch that shipping config inside the bundle exists to prevent. Also proven: rolling back to a release containing the probe still answered 404 until the restart, then 301. The container name moves to `CADDY_CONTAINER` rather than being hardcoded in the docs, since this repo names no host value in a committed file. `deploy/env.example` now states the naming convention, which is that the prefix names whatever owns the value rather than whatever reads it. HUGO_ is fixed by Hugo's own env mapping, DEPLOY_ is the release tooling that writes the root and is also the CI variable for the SSH deploy, and CADDY_ is container state a release never touches. The set was already consistent under that rule and nothing said so. Also clarifies the redirect table per review: the thirteen classes sum to 917 and `@uploads` is excluded deliberately, with each repeated `@mapped` row naming its map file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Correct the stale rule count in the two places it survived Re-deriving the count in deploy/README.md fixed one instance of it and left two, because the number was never grepped for across the repo. Both found by review. OPERATIONS.md still opened its Redirects section with "eleven regular-expression rules and five map files". It now states the verified figures and points at deploy/README.md for the per-class breakdown, so the two files stop restating each other and there is one place for a count to go stale. The blog post carried the same wrong figure, and a worse claim beside it: that rolling back "cannot leave yesterday's site being served by today's rules". That is exactly what a rollback does without a reload, as proven on the mirror earlier in this branch. A published post giving that advice would be actively misleading, so it now carries the catch, the way it was found, and the general lesson that an atomic swap is only atomic for whatever reads through it per request. Verified after the change with the full documented loop, release then restart then check: 1245 URLs honored. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Qualify when a missing restart actually invalidates the check The warning read as unconditional. It is not: a content-only release leaves the Caddyfile and the maps untouched, so the rules Caddy already holds are still the right ones and a check without a restart is honest. The false pass is specific to a release that changed `deploy/Caddyfile` or anything under `deploy/maps/`. The step stays unconditional regardless, and now says why rather than overstating the risk. Deciding correctly means knowing whether anything reached the config, which is easy to get wrong when a change spans several paths or a map was regenerated as a side effect, and getting it wrong is silent because the wrong answer is a green check rather than an error. A few seconds of restart is cheaper than that reasoning. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI proves the render half of the URL contract and cannot prove the other 917. A redirect is the web server's job, and the validation workflow has no server to point at, so a change to the Caddy config or a generated map is invisible to it. The workflow goes green while the redirect it broke stays broken until someone follows an old link.
OPERATIONS.mdgains a "Local Verification Before a Pull Request" section naming which paths require a local release and live check first, and why each one does:deploy/Caddyfiledeploy/maps/content/,static/hugo.yaml,layouts/The loop was run, not written from memory. Against the current build it reports
PASS - 1245 URLs honored, which is the output quoted in the new section.That also settles a residual delta in the audit, which recorded the redirect half as asserted rather than proven. It is now proven against a running server, though only the local mirror and only by hand. CI still cannot enforce it, and that stays true until staging exists, so the report says so rather than claiming the gap is closed.