Let CI assemble a release, and run the script that assembles it - #79
Conversation
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) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a staging deploy regression by making deploy/make-release.sh compatible with CI’s mtime-restore mechanism and by updating CI validation to actually execute the release-assembly path (not just lint it), so caller-contract regressions fail in PRs rather than at deploy time.
Changes:
- Add
MTIME_RESTORED=1knob so callers that already restoredstatic/mtimes can skip the script’s internal restore step while still enforcing the mtime assertion. - Update
validate-task.ymlto restore mtimes and rundeploy/make-release.shduring validation (with full git history). - Propagate the new knob through docs and the environment-doc consistency checker, and set it in the deploy workflow when calling
make-release.sh.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ENVIRONMENT.md | Documents the new MTIME_RESTORED per-invocation knob and its CI rationale. |
| deploy/make-release.sh | Allows skipping the mtime-restore tool requirement when mtimes were already restored, while keeping the assertion. |
| checks/check-env-docs.py | Adds MTIME_RESTORED to the knob list so docs/lint stay in sync. |
| .github/workflows/validate-task.yml | Adds full-history checkout, restores mtimes, and runs make-release.sh to validate the deploy path in CI. |
| .github/workflows/deploy-site-task.yml | Sets MTIME_RESTORED=1 when calling make-release.sh after restoring mtimes via the pinned action. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/workflows/validate-task.yml:117
- This job already runs
hugo --gc --minify --panicOnWarningandchecks/check-url-parity.py publicearlier, anddeploy/make-release.shruns both again (itrm -rf public, rebuilds, and re-runs the URL parity check). That doubles the most expensive work in the validation gate and will noticeably increase CI time.
Consider making the release-assembly step the single build/URL-contract gate (removing the earlier build + URL parity steps), or adding an explicit skip knob to make-release.sh for CI validation so it can reuse the already-built public/ output.
# The release script was linted here and run only by a deploy, so a broken caller contract reached production.
# Assembling into an empty scratch root exercises the deploy's path short of the transport.
# No previous release exists there, so the hard-link guard does not apply.
# REQUIRE_BROTLI stays unset, since installing brotli would cost an apt round trip per pull request.
- name: Assemble release bundle step
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) <noreply@anthropic.com>
|
Fixed in 1988159, taking your first option.
Correct, and measured rather than estimated — the same job on the previous branch against this one: Ten seconds of doing the two most expensive things twice, for no additional coverage. Why the first option and not the secondThe standalone build and URL-contract steps are removed, and the script is now the gate. It runs the identical commands — I deliberately did not take the skip-knob option. It would make There is a real cost to the option I did take, worth naming: a failure is now attributed to "Assemble release bundle step" whether it was hugo, the URL contract, or the script itself. That is one step instead of three, and the log distinguishes them, so I think it is the right trade — but it is a trade rather than free. |
|
Correcting the numbers I posted two comments ago. The change was right; my explanation of why was wrong, and I only found out because the follow-up measurement contradicted it. I said the duplication cost ten seconds, citing 38s → 48s. After removing it the job took 54s, which is the opposite of what that claim predicts. Per-step durations show what was actually going on:
So:
Removing the duplicate still stands on its own — the job did build twice, and now builds once — but it saved two or three seconds rather than ten, and I should not have asserted otherwise from a single pair of job totals. Validation is ~15s slower than before this PR, deliberately, and that is the honest trade: it now executes the script that assembles a release instead of only linting it. |
Fixes the staging deploy failure, and closes the gap that let it reach a deploy at all.
The regression
make-release.shhas two callers and my last change only considered one. It began requiringgit-restore-mtimeonPATH, which CI cannot satisfy — the workflows restore with a pinned action that runs the tool from the action's own directory.Nothing was uploaded and nothing flipped — the failure landed before the deploy key was installed.
MTIME_RESTOREDlets a caller that already restored skip the restore. It skips the work and never the assertion, so a caller that sets it without having restored fails the same check as one that never tried. Verified all three ways, against aPATHcarrying hugo and no mtime tool:Why it reached a deploy — the part worth fixing
validate-task.ymllintedmake-release.shand never ran it. The only thing that executed it was a dispatch-only workflow, so a broken caller contract was invisible to every 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, exercising the deploy's path short of the transport — the mtime contract, the build, the URL gate, the precompression, the staging tree, 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
Cut back to the house style in the touched files: one sentence per line, no wrapped prose, no incident narrative. Several had grown across the day's edits, which is the creep
GOVERNANCE.mdnames.