From 4dc3348fbf7b75966fcaa60694a8748177152406 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 9 Aug 2026 08:51:06 -0700 Subject: [PATCH] Let CI assemble a release, and run the script that assembles it (#79) * Let CI assemble a release, and run the script that assembles it make-release.sh has two callers and the last change only considered one. It began requiring git-restore-mtime on PATH, which CI cannot satisfy: the workflows restore with a pinned action that runs the tool from the action's own directory. The staging deploy failed at the assemble step with the tool genuinely absent, before any transport, so nothing was uploaded and nothing flipped. MTIME_RESTORED lets a caller that restored already skip the restore. It skips the work and never the assertion, so a caller setting it without having restored fails the same check as one that never tried. Verified all three ways, with a PATH carrying hugo and no mtime tool: knob set, tool absent, tree restored installs, exit 0 no knob, tool absent the failure that broke the deploy knob set, tree NOT restored assertion fails, exit 1 The second half is why this reached a deploy at all. validate-task.yml linted make-release.sh and never ran it, so the only thing that executed it was a dispatch-only workflow, and a broken caller contract could not be seen by any pull request. shellcheck cannot see that class: the defect was in what the script demanded of its caller, not in its syntax. Validation now assembles a release into a scratch root, which exercises the deploy's path short of the transport: the mtime contract, the build, the URL gate, the precompression, the staging tree and the release stamp. The root is empty, so no previous release exists and the hard-link guard does not apply. That step reproduces case A above, so this specific defect now fails on the pull request that introduces it. Its checkout takes fetch-depth: 0, since the restore has no commit to date a file from without history. Comments in the touched files are cut back to the house style: one sentence per line, no wrapped prose, no incident narrative. Several had grown across the day's edits, which is the creep the style rule names. Co-Authored-By: Claude Opus 5 (1M context) * Name what MTIME_RESTORED skips, and what it does not The comment, the log line and the reference row all said the knob skips the work and not the assertion, which reads as a distinction without a stated boundary. Each now says the two effects plainly: no restore is run and the tool is not required, while the assertion runs regardless. The log line said the mtimes were restored by the caller, which describes a state rather than the decision being taken. It now says the restore is being skipped and the assertion still follows. Found by Copilot review on #79. Co-Authored-By: Claude Opus 5 (1M context) * Make the release assembly the build gate, not a second one The assemble step re-ran the two most expensive things the job had already done: make-release.sh does rm -rf public, rebuilds with the same --panicOnWarning, and re-runs check-url-parity. Measured on this branch, the job went from 38s to 48s for no additional coverage. The standalone build and URL-contract steps are removed and the script is the gate. It runs the identical commands, so nothing is lost, and the reasoning that belonged to those steps moves to where the work now happens. The alternative was a knob letting the script reuse an existing public/ in CI. That was rejected: it makes the script behave differently under CI than locally, and a CI-only divergence in this script is precisely what broke the deploy this pull request is fixing. Found by Copilot review on #79, as a suppressed comment. Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) --- .github/workflows/deploy-site-task.yml | 52 +++------ .github/workflows/validate-task.yml | 30 ++++-- ENVIRONMENT.md | 1 + checks/check-env-docs.py | 1 + deploy/make-release.sh | 141 +++++++++++-------------- 5 files changed, 94 insertions(+), 131 deletions(-) diff --git a/.github/workflows/deploy-site-task.yml b/.github/workflows/deploy-site-task.yml index f0ca4a9..f20e229 100644 --- a/.github/workflows/deploy-site-task.yml +++ b/.github/workflows/deploy-site-task.yml @@ -75,57 +75,26 @@ jobs: with: fetch-depth: 0 - # One update for the job, because each one is a network round trip that can fail on its - # own. REQUIRE_BROTLI later makes a missing brotli fatal, so this keeps the build from - # failing. + # One update for the job, since each is a network round trip that can fail on its own. + # REQUIRE_BROTLI later makes a missing brotli fatal. - name: Install build tools step run: | set -Eeuo pipefail sudo apt-get update sudo apt-get install --yes --no-install-recommends brotli - # Git stores no mtimes, so a checkout stamps every file with the moment it was written. - # The deploy uploads with --link-dest against the previous release, and a file only links - # when size and mtime both match, so today nothing links and every release is a full copy. - # Restoring the last-commit time makes static/ match between releases: measured across two - # independent clones, all 1052 files land on identical mtimes, which is the same 1052 Hugo - # reports as static files and the same 1052 that link on a locally built release. - # - # static/ only. The generated pages are written fresh by every build and can never match, - # and walking the whole tree to prove that costs history reads for nothing. - # - # ORDERING: this is deliberately behind the live media check that #64 added. While every - # file arrives as a fresh inode, the upload re-asserts the mode contract on every deploy. - # Once 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 stays present, correctly named and - # unreadable, through every later release. The live check is what notices that, by - # requesting images and failing on the 403. - # The action rather than the Ubuntu package, because the package is git-tools v2022.12 and - # that release shells out to `git whatchanged`, which current git refuses to run without - # `--i-still-use-this` - a flag there is no way to pass through. It failed twelve times in - # one step, restored nothing, and exited 0, so every release since #65 was a full copy - # while CI reported success. Upstream replaced whatchanged with `git log` in v2025.08, and - # this action vendors exactly that version. + # A file links under --link-dest only when size and mtime both match, and git stores no mtimes. + # static/ only, since generated pages are written fresh by every build and can never match. + # The action rather than the Ubuntu package, whose git-tools v2022.12 calls `git whatchanged`. + # Current git refuses that and the tool exits 0 regardless, restoring nothing. - name: Restore file mtimes step uses: chetan/git-restore-mtime-action@d186aca54f8760da4dec55313195e51ed3ebb0b3 # v2.3 with: args: static - # The step above is the second tool to claim it restored these and not have done it, so - # the outcome is asserted rather than the tool trusted. This is the check whose absence - # let #65 ship broken: `git restore-mtime` printed "1,052 files to be processed" and then - # processed none of them, which reads exactly like success. - # - # The discriminator is that a restored file cannot be newer than the commit it was dated - # from, so no file under static/ may be newer than HEAD's own commit time. A checkout - # necessarily happens after the commit it checks out, so an unrestored tree is always - # newer than that bound and a restored one never is. It calibrates itself from the - # repository, so no measured constant goes stale as content moves. - # - # Counting distinct mtime days looks like the obvious check and is wrong: git restores - # the LAST COMMIT time, and static/ arrived in a bulk import, so a correctly restored - # tree here has exactly one distinct day. That version was written first and rejected - # only because it was run against a real restored clone before being trusted. + # The outcome is asserted because a failing restore reports success and does nothing. + # A restored file cannot be newer than the commit it was dated from. + # A checkout happens after the commit it checks out, so the bound calibrates itself. - name: Assert mtimes were restored step run: | set -Eeuo pipefail @@ -151,10 +120,13 @@ jobs: # Assembled to a scratch path, since the environment's deploy root is on the far host. # Naming the root explicitly also marks this a bundle for shipping rather than an install. + # The action above restored the mtimes and runs the tool from its own directory, not PATH. + # MTIME_RESTORED therefore skips the script's own restore, which would look for a tool absent here. - name: Assemble release bundle step env: HUGO_BASEURL: ${{ vars.HUGO_BASEURL }} REQUIRE_BROTLI: '1' + MTIME_RESTORED: '1' run: | set -Eeuo pipefail deploy/make-release.sh "${RUNNER_TEMP}/bundle" "${{ steps.release.outputs.id }}" diff --git a/.github/workflows/validate-task.yml b/.github/workflows/validate-task.yml index 49b2fba..9d971d5 100644 --- a/.github/workflows/validate-task.yml +++ b/.github/workflows/validate-task.yml @@ -17,8 +17,11 @@ jobs: steps: + # Full history, because the mtime restore below has no commit to date a file from without it. - name: Checkout code step uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + fetch-depth: 0 # Doc linters run as pinned action wrappers. # The editorconfig-checker action is install-only, so it runs via Docker instead. @@ -89,14 +92,23 @@ jobs: - name: Install Hugo step uses: ./.github/actions/install-hugo - # --panicOnWarning is the real gate. - # Two PaperMod templates are overridden in layouts/ precisely so it can stay on. - - name: Build site step - run: | - set -Eeuo pipefail - hugo --gc --minify --panicOnWarning + # The same action the deploy uses, so the two agree on who restores the mtimes. + - name: Restore file mtimes step + uses: chetan/git-restore-mtime-action@d186aca54f8760da4dec55313195e51ed3ebb0b3 # v2.3 + with: + args: static - # The URL contract is this repo's reason to exist, so a build that drops a legacy URL fails here. + # This is the build and URL-contract gate, not an extra step beside one. + # The script runs `hugo --panicOnWarning` and check-url-parity itself, so a separate build would repeat both. + # --panicOnWarning is the real gate, and two PaperMod templates are overridden in layouts/ so it can stay on. # 328 URLs must render, 778 legacy image URLs must resolve, and every local asset reference must exist. - - name: Check URL contract step - run: python3 checks/check-url-parity.py public + # + # Running the script here is also the point: it was linted and never run, so a broken caller contract reached a deploy. + # An empty scratch root exercises the deploy's path short of the transport, with no previous release for the hard-link guard. + # REQUIRE_BROTLI stays unset, since installing brotli would cost an apt round trip per pull request. + - name: Assemble release bundle step + env: + MTIME_RESTORED: '1' + run: | + set -Eeuo pipefail + deploy/make-release.sh "${RUNNER_TEMP}/validate-bundle" "$(date -u +%Y%m%d-%H%M%S)" diff --git a/ENVIRONMENT.md b/ENVIRONMENT.md index 700d172..e39b85f 100644 --- a/ENVIRONMENT.md +++ b/ENVIRONMENT.md @@ -89,6 +89,7 @@ Set on the command line for one run rather than stored anywhere. | `NO_LINK_DEST=1` | full copy instead of hard-linking from the previous release | | `KEEP_RELEASES` | how many releases `make-release.sh` leaves behind | | `EXPECT_RELEASE` | the release id `check-live-urls.sh` requires the live site to report, which is what makes a rollback verifiable rather than merely exiting zero | +| `MTIME_RESTORED=1` | the caller has already restored `static/` mtimes. Two effects, and no others: `make-release.sh` does not run the restore, and does not require `git-restore-mtime` to be installed. **The assertion that the mtimes are correct still runs**, so setting this without having restored fails the build rather than bypassing it. **CI sets it**, because the workflows restore with a pinned action that runs the tool from the action's own directory and never puts it on `PATH` | | `CHECK_TAG` | the `X-Blog-Check` provenance this run announces on every request. **`/` is enforced, not merely expected**: exactly one `/`, which is the separator and the only one allowed, with both halves non-empty and each drawn from letters, digits, `.`, `_`, `-`. Rarely set by hand, since `check-live-urls.sh` derives `github/-` under Actions and `proxmox/manual` elsewhere. Set it to name a purpose for a hand run, as `proxmox/media-dev` | ## Two credentials to the VPS, and why they are separate diff --git a/checks/check-env-docs.py b/checks/check-env-docs.py index e22891d..acc40e5 100755 --- a/checks/check-env-docs.py +++ b/checks/check-env-docs.py @@ -47,6 +47,7 @@ "KEEP_RELEASES", "EXPECT_RELEASE", "CHECK_TAG", + "MTIME_RESTORED", } # Names that look like configuration to the patterns above but are not. diff --git a/deploy/make-release.sh b/deploy/make-release.sh index 170471c..45f6b8a 100755 --- a/deploy/make-release.sh +++ b/deploy/make-release.sh @@ -86,94 +86,71 @@ command -v hugo >/dev/null || { cd "$REPO" -# Git stores no mtimes, so a checkout stamps every file with the moment it was written, and -# a release built from a fresh clone then links nothing against the previous one. This host's -# long-lived working tree has old mtimes already and links fine, which is exactly what makes -# the gap easy to miss: it is invisible here and total in a clean checkout. -# -# The deploy workflow does the same thing with the same assertion after it, deliberately, so -# the local path and CI fail the same way for the same reason rather than one of them being -# the trusted one. -# -# Required rather than optional. Skipping when absent is how the CI version shipped broken -# for four releases: it printed a reassuring line and restored nothing. -# Both invocation forms are accepted, because how it installs decides which one resolves. The -# Debian and Ubuntu package puts it in git's exec-path at /usr/lib/git-core, where only the -# subcommand form works; a manual install to /usr/local/bin gives the bare name and no -# subcommand. Testing only one would refuse a correctly installed tool. -# -# Each candidate is version-checked and the first ACCEPTABLE one wins, rather than the first -# one that merely exists. A host can carry both, and an old manual install must not veto a -# current packaged one sitting behind it. -# -# The version is gated rather than left to the assertion below, because before MTIME_MIN the -# tool calls `git whatchanged`, which current git refuses to run, so it reports files to be -# processed, processes none, and exits 0. Refusing it here names the cause; the assertion can -# only report the symptom. Versions are YYYY.MM, so dropping the dot compares them as integers. -mtime_probe() { - # The failed match is tolerated because `set -e` with `pipefail` would otherwise abort the - # whole script at the assignment, making every diagnostic below unreachable. - "$@" --version 2>/dev/null | grep -oE '[0-9]{4}\.[0-9]{2}' | head -1 || true -} - -MTIME_CMD=() -mtime_version="" -mtime_found="" -for mtime_form in bare subcommand; do - mtime_try=() - case "$mtime_form" in - bare) command -v git-restore-mtime >/dev/null 2>&1 && mtime_try=(git-restore-mtime) ;; - subcommand) git restore-mtime --version >/dev/null 2>&1 && mtime_try=(git restore-mtime) ;; - esac - [ ${#mtime_try[@]} -gt 0 ] || continue +# Git stores no mtimes, so a checkout stamps every file with the moment it was written. +# Without restoring them a release links nothing against the previous one. +# MTIME_RESTORED skips only the restore, for a caller that has already done it. +# The assertion below always runs, so a caller claiming this wrongly still fails. +if [ "${MTIME_RESTORED:-0}" = 1 ]; then + echo "==> skipping the restore: MTIME_RESTORED says the caller did it, still asserting below" +else + # How the tool was installed decides which invocation form resolves, so both are tried. + # The first form meeting MTIME_MIN wins, since a stale install must not veto a current one. + # Below MTIME_MIN it calls `git whatchanged`, which current git refuses, and it exits 0 anyway. + # Versions are YYYY.MM, so dropping the dot compares them as integers. + mtime_probe() { + # An unmatched grep would abort the script here, leaving the checks below unreachable. + "$@" --version 2>/dev/null | grep -oE '[0-9]{4}\.[0-9]{2}' | head -1 || true + } + + MTIME_CMD=() + mtime_version="" + mtime_found="" + for mtime_form in bare subcommand; do + mtime_try=() + case "$mtime_form" in + bare) command -v git-restore-mtime >/dev/null 2>&1 && mtime_try=(git-restore-mtime) ;; + subcommand) git restore-mtime --version >/dev/null 2>&1 && mtime_try=(git restore-mtime) ;; + esac + [ ${#mtime_try[@]} -gt 0 ] || continue + + mtime_try_version="$(mtime_probe "${mtime_try[@]}")" + if [ -z "$mtime_try_version" ]; then + mtime_found="${mtime_found}${mtime_found:+, }${mtime_try[*]} (no version reported)" + continue + fi + mtime_found="${mtime_found}${mtime_found:+, }${mtime_try[*]} $mtime_try_version" + if [ "${mtime_try_version//./}" -ge "${MTIME_MIN//./}" ]; then + MTIME_CMD=("${mtime_try[@]}") + mtime_version="$mtime_try_version" + break + fi + done - mtime_try_version="$(mtime_probe "${mtime_try[@]}")" - if [ -z "$mtime_try_version" ]; then - mtime_found="${mtime_found}${mtime_found:+, }${mtime_try[*]} (no version reported)" - continue - fi - mtime_found="${mtime_found}${mtime_found:+, }${mtime_try[*]} $mtime_try_version" - if [ "${mtime_try_version//./}" -ge "${MTIME_MIN//./}" ]; then - MTIME_CMD=("${mtime_try[@]}") - mtime_version="$mtime_try_version" - break - fi -done - -if [ ${#MTIME_CMD[@]} -eq 0 ]; then - if [ -z "$mtime_found" ]; then - echo "git-restore-mtime not found, as either 'git-restore-mtime' or 'git restore-mtime'" >&2 - else - echo "no usable git-restore-mtime: found $mtime_found, and $MTIME_MIN or newer is required" >&2 - echo " before $MTIME_MIN it calls 'git whatchanged', which current git refuses to run, so it" >&2 - echo " restores nothing and still exits 0 -- every release would silently be a full copy" >&2 + if [ ${#MTIME_CMD[@]} -eq 0 ]; then + if [ -z "$mtime_found" ]; then + echo "git-restore-mtime not found, as either 'git-restore-mtime' or 'git restore-mtime'" >&2 + else + echo "no usable git-restore-mtime: found $mtime_found, and $MTIME_MIN or newer is required" >&2 + echo " before $MTIME_MIN it calls 'git whatchanged', which current git refuses to run, so it" >&2 + echo " restores nothing and still exits 0 -- every release would silently be a full copy" >&2 + fi + echo " it is what makes --link-dest able to link, and a release built without it is a full copy" >&2 + echo " install git-tools $MTIME_MIN or newer, from https://github.com/MestreLion/git-tools" >&2 + exit 1 fi - echo " it is what makes --link-dest able to link, and a release built without it is a full copy" >&2 - echo " install git-tools $MTIME_MIN or newer, from https://github.com/MestreLion/git-tools" >&2 - exit 1 + + echo "==> restoring file mtimes with ${MTIME_CMD[*]} $mtime_version" + "${MTIME_CMD[@]}" static fi -echo "==> restoring file mtimes with ${MTIME_CMD[*]} $mtime_version" -"${MTIME_CMD[@]}" static - -# Asserted rather than trusted, because the failure this exists for is a restore that reports -# success and does nothing. A restored file cannot be newer than the commit it was dated from, -# so nothing under static/ may be newer than HEAD's commit time. -# -# Locally modified files are excluded, which is the one way this differs from CI. A working -# tree can legitimately hold a static file newer than any commit; a fresh CI checkout cannot, -# so there the same check needs no exclusion. Comparing the clean files only keeps the -# assertion meaningful during an edit loop instead of being skipped whenever the tree is dirty. +# The restore is asserted because a failing one reports success and does nothing. +# A restored file cannot be newer than the commit it was dated from. +# Uncommitted paths are excluded, since a working tree may hold one newer than any commit. mtime_bound="$(git log -1 --format=%ct)" -# `git status --porcelain` covers modified, staged and untracked in one list, so an empty -# result means every file under static/ is tracked and unchanged. That is the CI case, and it -# takes the same one-pass `find` the workflow uses. -# A rename or a copy emits TWO NUL records, `XY ` then a bare ``, so the loop has to -# consume the second explicitly. Reading it as another status record would strip three -# characters off a bare path and record `tic/a.txt` for `static/a.txt`, leaving the real path -# unexcluded and the assertion able to fail on a file that is legitimately uncommitted. -# Both halves of a rename are excluded, since both are uncommitted. +# An empty porcelain list means every file under static/ is tracked and unchanged. +# A rename emits two NUL records, `XY ` then a bare ``, so the second is consumed here. +# Both halves are excluded, since both are uncommitted. declare -A mtime_dirty=() while IFS= read -r -d '' entry; do mtime_dirty["${entry:3}"]=1