Fetch media in the live check, so a lost image is caught somewhere - #64
Conversation
The build gate proves the media set against files on disk. Nothing proved those files reached the server or that the server can read them: the live check requested pages and redirects and never one image, so a media tree lost between a passing build and the server was caught by neither gate. On a site whose value is eighteen years of posts with images in them, that was the gap worth closing. checks/golden-media-live.txt is a handful rather than exhaustive, because the set is already proven and this is a delivery check. Its entries cover both trees, since they arrive by different routes and a partial transfer is unlikely to land evenly, plus the legacy /wp-content/uploads/ form, which nothing else exercised against a running server. Three assertions, each for a different loss. A missing file answers 404, a file whose mode went wrong answers 403, a file truncated to nothing still answers 200 so the byte count is checked, and a server answering an error page for a missing asset answers 200 as text/html so the content type is checked too. The 403 case is why this is not theoretical: a hard-linked file carries its inode's mode, so one that acquires a bad mode rides the chain into every later release, present and correctly named and unreadable, which a build-time is_file() cannot see. check_media follows one hop by hand rather than passing -L to curl, because -L would carry the auth-gate credential to wherever the rule points. Same origin boundary as check_redirect, for the same reason. Verified against production: PASS at 1253, and each failure shape reproduced rather than assumed. A missing image, a legacy URL redirecting to a missing image, a 200 that is not an image, and a zero-byte 200 all fail with the list entry named. The legacy entries deliberately went in this list rather than redirect-urls.txt. Adding them there would move a count stated in six documents and in the published migration post, which is disproportionate to three test URLs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a small, targeted set of live media URL checks to the existing “live URL contract” gate so that missing/unreadable/truncated images (or soft-404 HTML responses) are caught after deployment, not just at build time.
Changes:
- Introduces
checks/golden-media-live.txtas a short live-request list covering/media/,/external/, and legacy/wp-content/uploads/URLs. - Extends
checks/check-live-urls.shwith acheck_mediastep that asserts status, non-empty body, and image content type (with a redirect hop followed safely). - Documents the rationale and closes the corresponding TODO entry.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| TODO.md | Records the new live media check as completed and unblocks the mtime restoration follow-up item. |
| checks/README.md | Documents why a second (live) media list exists and what it asserts. |
| checks/golden-media-live.txt | Adds the curated live media URL list used by the live gate. |
| checks/check-live-urls.sh | Implements the new parallelized live media verification and includes it in totals/floors. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
`read ... < <(curl ...)` discards curl's exit status. Measured before acting on it: the check still fails closed, because curl writes 000 for http_code on a refused connection, a DNS failure and a timeout alike, so none of them can pass. What was lost was the diagnosis, since a bare "expected 200, got 000" leaves a reader to infer that no request completed at all. Command substitution preserves the status, so both are reported now: "no HTTP response: curl exit 7, transport error or timeout". The two older functions in this file use the same process-substitution shape and are left alone here, since they fail closed for the same reason and changing them is a separate change with its own testing. Raised by Copilot on PR #64. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
checks/check-live-urls.sh:104
check_mediaparses curl output withread -r code len type <<<"$out"while thecontent_typefield can legally contain spaces (e.g.text/html; charset=utf-8). With the current space-separated-wformat,typewill be truncated at the first space, which reduces diagnostic quality and can diverge from the expected failure messages described in the PR (e.g. it would reporttext/html;instead oftext/html; charset=utf-8). Use a delimiter that cannot appear in the content type (tab) and setIFSaccordingly when reading.
out=$(curl -s -o /dev/null \
-w '%{http_code} %{size_download} %{content_type}\n' \
--max-time 30 "${target_auth[@]}" "$target") || rc=$?
read -r code len type <<<"$out"
if [ "$rc" -ne 0 ] || [ "${code:-000}" = "000" ]; then
Declining the parsing change the review proposed, since `read` assigns the whole remainder of the line to its final variable and the type arrives intact, verified against both a literal and a live response. What the finding does surface is a latent edit hazard rather than a current defect: the value survives only because it is last, so a fourth field appended after it would be swallowed into the type. The constraint now sits on the format string, where someone would break it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
One suppressed finding, and this is the first one today I am declining. Measured rather than argued.
The premise about content types containing spaces is right. The consequence is not. $ out='200 1234 text/html; charset=utf-8'
$ read -r code len type <<<"$out"; printf 'type=[%s]\n' "$type"
type=[text/html; charset=utf-8]And against a real response rather than a literal: $ o=$(curl -s -o /dev/null -w '%{http_code} %{size_download} %{content_type}\n' https://<site>/about/)
$ read -r c l t <<<"$o"; printf 'type=[%s]\n' "$t"
type=[text/html; charset=utf-8]The failure message quoted in the PR description was captured from an actual run against production, not written by hand, which is the other reason it already shows the full type. Truncation would happen if the field order put |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
checks/README.md:72
- The README’s quick “check the counts” command doesn’t include the newly added
golden-media-live.txt, so following it won’t reflect all check lists currently in use.
wc -l checks/golden-urls.txt checks/redirect-urls.txt checks/golden-media-legacy.txt
The README's check-the-counts command names three lists and there are four, so following it silently omits the one this branch adds. A command that is meant to save you from trusting a written number should not itself be a written number that drifted. Verified the command now covers every list the live check reads. Raised by Copilot on PR #64. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Correct, and it is the one kind of staleness this file least affords. That command exists so a reader checks a count rather than trusting a written one, and it had itself become a written thing that drifted. Fixed in 3d6eb14, and I confirmed it now names every list |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
checks/check-live-urls.sh:23
- The FLOOR/truncation guard can be bypassed if
grepfails to read a list file (missing/unreadable). Because the script does not useset -e, a failingn=$(grep ...)can yield an emptyn, and the numeric test may error and evaluate false, allowing the script to continue and potentially run with an empty input set.
for list in golden-urls.txt redirect-urls.txt golden-media-live.txt; do
n=$(grep -c . "$CHECKS/$list")
if [ "$n" -lt "${FLOOR[$list]}" ]; then
echo "FAIL $list: $n URLs, expected at least ${FLOOR[$list]} - the list has been truncated" >&2
exit 1
checks/check-live-urls.sh:93
- When following a 301/308,
curl -w '%{redirect_url}'can return an empty string (e.g., missing/invalidLocationheader) or fail, which currently leads to a subsequentcurl "$target"with an empty/invalid URL and a misleading "transport error" message. Validate that a redirect target was actually obtained and fail with a clear message if not.
301 | 308)
target=$(curl -s -o /dev/null -w '%{redirect_url}' --max-time 30 "${auth[@]}" "$target")
# Same origin boundary as check_redirect, and for the same reason: a rule that one
# day points off-site must not mail the token there. A bare prefix would also accept
# a lookalike host registered as an attacker's subdomain.
Two suppressed findings, both real, and the first is the failure family this repo already documents: a query that matched nothing reading as a clean result. An unreadable list makes `grep -c` yield nothing, and `[ "" -lt N ]` is a syntax error that evaluates false, so the guard against a truncated list was itself skipped. Reproduced: the guard does not fire, the run continues with no URLs, and it exits 0. A guard that fails open is worse than no guard, and this one protected every assertion below it. Readability is asserted and the count is validated as a number before it is compared. Verified that a missing list and a truncated list now both exit 1 with distinct messages, and that a healthy run still passes. Second: a 301 carrying no usable Location left the follow-up target empty, and fetching an empty URL was reported as a transport error, which names the wrong problem. It now says what actually happened. Raised by Copilot on PR #64. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both real, both fixed in 5b92b46. The first is the more serious finding on this branch.
Correct, and reproduced exactly as described: n=[]
/bin/bash: line 15: [: : integer expression expected
GUARD DID NOT FIRE - script would continue
outer rc=0A guard that fails open is worse than no guard, and this one sits above every assertion in the file: the run would continue with an empty input set and exit 0, having checked nothing. That is the exact shape this repository has a standing rule about — a query that matched nothing reading as a clean result — so it is a good catch against code that was written by someone who knows the rule. Readability is now asserted before the count, and the count is validated as a number before it is compared. Verified all three paths: Worth noting the scope: this guard predates the branch and covered two lists before it covered three. The hole was pre-existing and this change is where it surfaced.
Correct. It still failed closed, but it named the wrong problem, and a message that misdirects costs more than a missing one when someone is reading a failed gate at speed. An empty target is now reported as |
The deploy uploads with --link-dest against the previous release, and a file links only when size and mtime both match. Git stores no mtimes, so a checkout stamps every file with the moment it was written and nothing ever linked: every release has been a full copy. Measured with two independent clones each way: without the restore 0 of 1791 files linked with the restore 1052 of 1791 files linked 1052 corroborates from three directions: it is what Hugo reports as static files, what links on a locally built release, and the file count in static/. Those files are 566 MB of the 586 MB a release occupies. Three things checked rather than assumed, any of which would have made this a no-op or a breakage. Hugo preserves a static file's mtime into public/, verified by touching a source and rebuilding. The restore is deterministic, with two clones producing byte-identical mtimes across all 1052 files, because static/ has stable last-commit times. And the Debian package installs into git's exec-path rather than onto PATH, so the subcommand form resolves and the bare binary name does not. static/ only. Generated pages are written fresh by every build and can never match. ORDERING: this follows the live media check merged in #64, deliberately. While every file arrived as a fresh inode the upload re-asserted the mode contract on every deploy. Now that a third of the tree arrives as hard links, a link carries the mode its inode chain began with, so a media file that acquires a bad one would stay present, correctly named and unreadable, through every later release. The live check is what notices that, by requesting images and failing on the 403. Not exercised here: an actual pipeline deploy. The first deploy after this merges is what proves the link count against the real host, where the host side has been measuring zero shared inodes.
The build gate proves the media set, against files on disk. Nothing proved those files reached the server or that the server can read them: the live check requested pages and redirects and never one image, so a media tree lost between a passing build and the server was caught by neither gate. On a site whose value is eighteen years of posts with images in them, that was the gap worth closing.
This also unblocks
git-restore-mtime, which was deliberately held behind it.What the list is, and why it is short
checks/golden-media-live.txtis a handful rather than exhaustive, because the set is already proven and this is a delivery check. The entries cover both trees, which arrive by different routes and where a partial transfer is unlikely to land evenly, plus the legacy form:/media/paths/external/paths/wp-content/uploads/paths@uploadsrule still lands on the image, which nothing else exercised against a running serverEvery entry was verified with a live request before being added, like every other list here.
Three assertions, because each catches a different loss
text/htmlThe 403 case is why this is not theoretical. A hard-linked file carries its inode's mode, so a media file that acquires a bad one rides the chain into every later release, present and correctly named and unreadable to the server. A build-time
is_file()on the runner cannot see it, and neither could a check that never requests an image.check_mediafollows one hop by hand rather than passing-Lto curl, because-Lwould carry the auth-gate credential to wherever the rule points. Same origin boundary ascheck_redirect, for the same reason.Verification
Against production,
PASS - 1253 URLs honored, with the media step running.Each failure shape was reproduced rather than assumed, and each names the list entry rather than the resolved URL:
The three legacy entries passing is itself the proof that the hop is followed: unfollowed they would have failed as
301.One judgment worth flagging
The legacy entries went in this list rather than
redirect-urls.txt, where they would also have worked. Adding them there moves a count stated in six documents and in the published migration post (328 + 917 = 1,245), which is disproportionate to three test URLs. The documented counts are unchanged: render 328, redirect 917.Gates
shellcheck and
shfmt -dclean on all four scripts, markdownlint 18 files clean, editorconfig clean,check-env-docs.py28/28, and the build gate unaffected. The hub'sprose_lint.pyis at its exact baseline for both files touched.