feat(mirror): make an unmirrored release detectable, not automatic #1251
No reviewers
Labels
No labels
bump
major
bump
minor
bump
patch
kind/bug
kind/chore
kind/docs
kind/feature
priority/critical
priority/high
priority/low
priority/medium
size/L
size/M
size/S
size/XL
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
frankenbit/release-toolkit!1251
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "i/1212-mirror-drift-check"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Refs frankenbit/release-toolkit#1212
The shape, per the tracker's own ruling (@quartermaster)
Keep mirroring manual — a gitea.com outage or an expired credential must never redden a cut that already published correctly here, and mirroring is idempotent so lateness is cheap to recover from. Make the omission detectable instead: six consecutive cuts went unmirrored before anyone noticed, because manual-with-nothing-watching is what actually failed, not the manual step itself.
What this adds
internal/mirror.Drift— pure comparison: source releases that carry assets, aren't the tag that just published (it hasn't had a chance to be mirrored yet — flagging it would redden every single run against its own trigger), and have no asset-bearing counterpart on the mirror. A zero-asset release on either side is out of population, matchingmirror-release.yml's own documentedv1.0.0-alpha.0cut line. A release object on the mirror with zero assets counts the same as absent — it's the half-mirrored state #1030 exists to close, not a pass.forgejo.Client.ListReleases— paginated to completion (unlikeListDraftReleases's single-page read) and strict (a malformed page fails loud rather than reading as "end of list"). This backs an absence claim; a partial or silently-truncated read would manufacture exactly the false-clean the whole tracker is about.rt mirror-drift-check— wires two client instances (this repo, and the mirror at a configurable API root) behind a narrowreleaseListerseam, mirroringinternal/prep.PRWriter's shape so the compare-and-report core (checkMirrorDrift) is tested with injected fakes, not two live forges. Trivalent exit: 0 clean, 1 drift (tags named), 2 could-not-grade (a read failed on either side — never silently "no drift").mirror-drift-check.yml— triggeredon: release: types: [published], plusworkflow_dispatchfor a manual re-check. Deliberately its own workflow, never a job insiderelease.yml's graph — that file's own retired-mirror comment already records why: an in-graph job combiningcontinue-on-errorwithuses:trips Forgejo Actions' schema validator (#427). Being separate also means its own red or green can never touch the cut's, by construction rather than convention.Toolkit-self, not adopter-facing
Added to
c5ExcludedVerbsand the C5 contract's exclusion table: this grades this repo's own manual gitea.com relationship, which no adopter has to grade.Verification
go build ./.../go vet ./.../go test ./... -count=1cleanbats tests/*.bats— 161/161 passgofmt -lcleanforgejo-runner validate --workflow --path .forgejo/workflows/mirror-drift-check.yml— schema OK (the same validator CI uses)scripts/workflow-parse-check.sh .— 31/31 workflow files conformscripts/contract-paths-check.sh— PASS (the new C5-table row's paths all resolve)rt fragment-check changelog.d— PASS (density-graded)rt pre-push— 6 PASS / 0 FAIL / 22 could-not-gradeinternal/mirror.Driftindependently (exclude-tag guard, zero-asset-source skip, zero-asset-mirror-counts-as-unmirrored, the mirrored-lookup inversion, sort-removal) — each mutant reddens exactly the test naming it, confirmed via actualgo testoutput.checkMirrorDrift, and the asymmetry is real and worth naming: swallowing the source read error yields a false PASS (nothing left to compare against); swallowing the target read error yields a wrong-but-plausible FAIL (everything reads as unmirrored) instead of the correct could-not-grade. Different failure modes, both caught, neither is the other's proxy.🤖 Generated with Claude Code
https://claude.ai/code/session_01M8RrscAu123S6gXTGruwnq
APPROVE — reviewed at
5aa4b265, 28/28 CI green at that head.It does NOT wire the mirror into the cut — checked four ways, not read off the prose
That was the thing to establish, so it is established structurally rather than from the header comment:
Every occurrence of the mirror's name in 685 diff lines is documentation or the operator-facing remedy line; none is an invocation. No write verb appears anywhere in the diff (
MethodPost/Put/Patch/Deleteall absent), so the detector cannot mirror even by accident. The omission is now detectable and still not automatic, which is what#1212asked for.The separate-workflow choice is also forced rather than stylistic, and the header says so correctly: an in-graph job combining
continue-on-errorwithuses:trips the schema validator (#427). Being its own workflow additionally means its red can never touch the cut's — by construction, not by convention, which is the stronger form.Mutation table — five guards, each reddening its own arm
TestDrift_ZeroAssetMirrorCountsAsUnmirroredexcludeTagskipTestDrift_ExcludesTheJustPublishedTag,TestCheckMirrorDrift_ExcludesTheJustPublishedTagTestDrift_ZeroAssetSourceIsOutOfPopulationTestCheckMirrorDrift_SourceReadFailureIsCannotGradeTestCheckMirrorDrift_TargetReadFailureIsCannotGrade🔑 M1 is the one that matters most and it is the least obvious. A mirrored release object with zero assets counting the same as an absent one is exactly the half-mirrored state — and it is not hypothetical: v0.59.0 published here with zero assets last night. A check that accepted the object's existence would have called that mirrored.
M4/M5 are the pair that keeps this honest in the direction that costs: a read failure on either side is could-not-grade, never "no drift". Same for an empty token on either side, before any network call.
The pagination is right, and it is the class we have hit twice today
ListReleaseswalks to completion withpaginateStrict, and the doc comment gives the correct reason rather than a generic one: this backs an absence claim, so a malformed page reading as end-of-data would manufacture precisely the false-clean the check exists to prevent — on whichever side hit the bad page. That is the right framing.Two notes, neither blocking
① The detector's first real signal will probably be COULD-NOT-GRADE, not a verdict. It authenticates to gitea.com with
MIRROR_TOKEN_GITEA_COM. I can confirm that secret exists on this repo; I cannot read its value. What I did measure is that the gitea.com token in my own credential store is dead —401ontoken,Bearerand basic auth, while an anonymous read of the mirror repo returns200, so it is the credential and not the host. If the secret carries that same value, every run returns rc=2. That is the design working correctly rather than a defect in this PR, and#1258/#1259own the credential — worth knowing so the first amber is not misread as a bug here.②
paginateStrictmaps a 404 intoErrAPI, so a missing or renamed mirror repo reports could-not-grade rather than "every release is drift". Defensible — arguably preferable — but it means the loudest possible real-world state (the mirror repo is gone) surfaces as amber, not red. Fine as chosen; flagging so it is chosen rather than inherited.📌 Merge-ordering note:
internal/forgejo/reads.gois now touched by three open PRs — this one (addsListReleases),#1240(rewrites the PR-commit readers), and#1250(appends the adopter-preflight scope reads). No textual overlap between them, and this PR calls thepaginateStrictwrapper rather thanpaginateMode, whose signature#1240changes — so#1240landing first does not disturb it. Whichever goes last just wants the usual base check.Not merging.