fix(#931): stop the manifest test clobbering the tracked artifact, and retire a fossil - #948
Merged
Merged
Conversation
…d retire a fossil
Two defects, one file. The reported symptom -- a permanently dirty working tree --
was the smaller of them.
THE TEST DISCARDED UNCOMMITTED WORK. `generate-manifest.js` resolved its output only
against __dirname, so the test covering it could not run the generator without
overwriting public/manifest.json. It compensated in a finally block with
git checkout -- public/manifest.json
which resets to HEAD, not to the pre-test working state. Running the suite therefore
threw away any uncommitted change to that file. This was not deduced: it destroyed
the fix below while that fix was in progress, and the second attempt is what exposed
it. The generator now honours MANIFEST_OUTPUT_DIR, the test writes into its fixture,
and the destructive restore is gone.
THE COMMITTED MANIFEST WAS A FOSSIL. detect-project.js:132 gives a non-empty base
path only when `isGitHubActions && info.isGitHub && !cnameExists`. This repo has
public/CNAME, so every build -- local and deploy -- produces a base path of '' and a
start_url of '/'. Production agrees: scripthammer.com/manifest.json serves
"start_url": "/". The committed copy said "/ScriptHammer/", a value the deploy has
not produced since the custom domain was added and which nothing serves.
generated-manifest.test.js was pinning that fossil, because defaultBasePath()
returned `/<projectName>` unconditionally and ignored the CNAME clause. Hence both
symptoms from one stale expectation: every build rewrote the file, so the tree was
never clean, and committing the CORRECT regenerated value failed the test.
defaultBasePath() now replicates the CNAME clause, and the committed manifest is
regenerated to match what builds and production actually produce. It is idempotent:
regenerating twice leaves no diff.
`isGitHubActions` is deliberately NOT replicated. The committed artifact must
represent what the DEPLOY produces, and the deploy always runs in Actions -- keying
off the local environment would make the expectation flip depending on where the
suite happened to run.
THE FORK PROTECTION IS INTACT, which is the point of the test. Removing public/CNAME
-- a fork on github.io project pages -- makes the expectation `/ScriptHammer` again,
so a DISABLE_BASE_PATH=true build committed there still fails. Verified by moving the
file aside: 2 pass / 1 fail without it, 4 pass / 0 fail with it.
Adds a guard for the behaviour that made this file dangerous: the generator must
honour MANIFEST_OUTPUT_DIR and must not touch the tracked artifact while doing it.
Mutation-tested -- reverting the generator to its __dirname-only form fails two tests.
NOT PART OF THIS, and my earlier claim was too broad: public/robots.txt was NOT
churning because of a repo defect. Its committed value already matches production
(https://scripthammer.com/sitemap.xml). It regenerated to the github.io fallback only
in a worktree whose .env lacks NEXT_PUBLIC_DEPLOY_URL, which is a property of that
worktree (#932), not of the generator.
Verified: 566/566 script tests, type-check clean, gitleaks clean, and a full suite run
no longer modifies public/.
Closes #931
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #931.
Two defects, one file. The reported symptom — a permanently dirty working tree — was the smaller of them.
The test was silently discarding uncommitted work
generate-manifest.jsresolved its output only against__dirname, so the test covering it could not run the generator without overwritingpublic/manifest.json. It compensated in afinallyblock:That resets to HEAD, not to the pre-test working state. So running the suite threw away any uncommitted change to that file.
This was not deduced. It destroyed the fix below while that fix was in progress, and the second attempt is what exposed it. Anyone editing that file and running the suite loses the edit, with no error.
Fixed by making the generator honour
MANIFEST_OUTPUT_DIR. The test now writes into its fixture and the destructive restore is gone.The committed manifest was a fossil
detect-project.js:132gives a non-empty base path only whenisGitHubActions && info.isGitHub && !cnameExists. This repo haspublic/CNAME, so every build — local and deploy — produces''and astart_urlof/.Production agrees:
start_urlscripthammer.com/manifest.json//ScriptHammer/A value the deploy has not produced since the custom domain was added, and which nothing serves.
generated-manifest.test.jswas pinning it, becausedefaultBasePath()returned/<projectName>unconditionally and ignored the CNAME clause.Hence both symptoms from one stale expectation: every build rewrote the file, so the tree was never clean, and committing the correct regenerated value failed the test.
The committed manifest is regenerated to match reality, and it is idempotent — regenerating twice leaves no diff.
isGitHubActionsis deliberately not replicated in the test. The committed artifact must represent what the deploy produces, and the deploy always runs in Actions; keying off the local environment would make the expectation flip depending on where the suite ran.The fork protection is intact — that is the point of the test
Removing
public/CNAME(a fork on github.io project pages) restores the/ScriptHammerexpectation, so aDISABLE_BASE_PATH=truebuild committed there still fails.New guard, mutation-tested
Added: the generator must honour
MANIFEST_OUTPUT_DIRand must not touch the tracked artifact while doing it — the guard for the behaviour that made this file dangerous. Reverting the generator to its__dirname-only form fails two tests.A correction, stated rather than quietly dropped
I originally filed #931 blaming the generators for both
manifest.jsonandrobots.txt. The robots half was wrong. Its committed value already matches production (https://scripthammer.com/sitemap.xml). It regenerated to thegithub.iofallback only in a worktree whose.envlacksNEXT_PUBLIC_DEPLOY_URL— a property of that worktree (#932), not of the generator. Nothing about robots.txt is changed here.Verification