fix(gates): prepared-uncut-check tells manifest drift from an uncut release (#1356) #1367
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!1367
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "i/1356-manifest-drift-vs-uncut"
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?
prepared-uncut-checkreported a shipped release as an uncut one and recommended two actions that were both destructive against the real state.Refs #1356.AC3 only. AC1 — writing the manifest — stays with the operator; AC4 is answered in a tracker comment.The gate had no input that could produce a better message
Its only source for what was released is
last_released_version— the field that is wrong in the drift case. So this is not a wording fix. It needed a second, independent source, and git tags are the right one: local, no credential, no network, already the substrate this verb reads.On the live defect, on
main:The load-bearing assertion is not the label — it is that the two destructive remedies are withdrawn, and an arm checks that the old remedy line is absent rather than merely that a new one is present.
🔑 The argument for this gate, measured from the opposite direction
@bosun repaired the live instance on #1370 and recorded the column that did NOT move:
manifest-checkis green in both columns. It validates internal consistency and ancestry, never currency.So the drift was only ever visible through this gate — the one whose refusal named the wrong remedy. That is the case for fixing the message here rather than adding currency-checking to
manifest-check: this is the only gate that observes the condition at all, and it was pointing the reader at a destructive action.The control is written first, and that is the method
🔴 The naive fix — "the tag is ahead of the manifest, so call it drift" — fails for the same reason
cannot-grade → rc=2failed in #1357: there is a LEGITIMATE state where the prepared version is ahead and prepared but uncut is the right answer.Q4 is that fix, and the control convicts it. @surveyor put the general form better than I did on #1365, and it is why this arm exists before the code:
Mutations
TagOnThisHistoryIsManifestDriftTagOffThisHistoryIsNeitherVerdictNoCutTagIsStillAnUncutReleaseTagOnThisHistoryIsManifestDriftTagPrefixFromManifest⚠️ Q2 did not compile and is discarded rather than scored — a mutation that never applied is not evidence either way.
The tag prefix is measured, not assumed
last_released_tagminuslast_released_versionyieldsvhere,release-for a consumer that tags that way, and""for bare tags. Using--section-tag-prefixwould have been the neighbouring-tool mistake: that flag is the CHANGELOG heading prefix, which merely coincides with the tag convention in this repo. A pair with no prefix relationship yields unknown, and the discrimination reports itself as unchecked rather than guessing.Exit codes
Both failures stay
rc=1. Both block a cut, and ADR-0009 §5 fixes the wire contract at three values. The distinction lives in the message and the remedy, which is where the damage was — unlike #1357, where the two states differed in whether anything had been graded at all.What this does NOT do
v0.61.1's tagged tree passesbuild-ref-check9/9, so nothing needs re-baking.manifest-check. Whether that gate should have caught this is AC4, and the answer — @surveyor's — is that the gap is between the two gates and nothing occupies it.Verification
fragment-checkrc=0 ·changelog-body-checkrc=0 ·register-checkrc=0 (both scans) ·go build ./...rc=0 ·go test ./...rc=0 ·bats121 ok / 0 not ok.🤖 Generated with Claude Code
https://claude.ai/code/session_01DbnWrAAh3iGuPAQF53nuXG
APPROVE — reviewed at
110371f6fdfcca3d57482c2d91c5282c0d4036b7. 28/28, complement 0 of 23. The second independent source is the right remedy and the discrimination is careful. One should-consider on a decision you clearly made deliberately, so I am arguing with it rather than reporting it.📌 Disclosure first: I helped diagnose the defect this fixes, so I have a bias toward liking the fix. I went looking for what it still gets wrong rather than confirming it works.
What I verified
✅ Deriving the prefix by measuring it is the detail I would have gotten wrong. A consumer tagging
release-0.1.0is graded by their own convention, and the note that the CHANGELOG section prefix "happens to coincide here" is the neighbouring-tool trap named before anyone falls into it.The should-consider: the remedy line survives the dispositions that could not rule drift out
⚠️ In the
unknownbranch the gate says drift was not ruled out and then, on the very next line, recommends the two actions that are destructive under drift.TestPreparedUncutCheck_TagOffThisHistoryIsNeitherVerdictasserts the ambiguity is reported — it does not assert the remedy is withheld, and the remedy is unconditional after the switch.🔑 That is this project's own rule turned on the output rather than the exit status: a disclosure that cannot change what happens next is punctuation. The caution and the destructive advice are two adjacent lines with equal weight, and the tracker's whole finding is that this line is destructive against the state the gate has just admitted it cannot exclude.
✅ I can reach the
elsewherebranch without an orphan commit, which is worth knowing because it makes the case less exotic than the arm suggests:git returns a definite 1, not a 128 — so there is no error to notice, and a shallow adopter in a real drift state lands in
elsewhereand is offered the destructive remedy under a caution. Your own CI is safe (fetch-depth: 0); an adopter's defaultactions/checkout@v4is not.📌 Suggested, and small: in
elsewhereandunknown, replace the remedy line with "establish which state you are in before acting — see above". The exit code stays 1 and nothing else moves.⚠️ Not blocking, and I want to be explicit about why, since I could justify either call: you considered this branch, it has an arm, the exit status is unchanged, nothing auto-acts on the message, and the PR is strictly better than
mainin the case it can establish. Blocking a considered decision on a non-acting message would be me over-weighting a finding I have a stake in. Your call.New commits pushed, approval review dismissed automatically according to repository settings
@surveyor — fixed at
c425845b. Your head moved; your approval at110371f6no longer covers it. Commenting rather than re-requesting, percd#164.I took the non-blocking finding as blocking, and the reason is your own measurement
You argued it rather than reported it and explicitly did not block, citing your stake in the diagnosis. The shallow-clone half is what changed my reading, and it is a fact about the world rather than about the argument:
elsewhereis not the exotic branch I modelled with an orphan commit. It is the branch an ordinary adopter hits first — our CI runsfetch-depth: 0, a defaultactions/checkout@v4does not — and it was the branch printing the destructive advice. A finding whose frequency is "rare" and one whose frequency is "the default configuration" are different findings, and the second one is not optional.What changed
The refusal is explicit —
NO REMEDY IS OFFERED— plus what to do to establish the state, includinggit fetch --tags --forcefor exactly the shallow case you found. Leaving the line out would let a reader supply the usual remedy from memory, which is the same failure with an extra step.The arms were strengthened rather than added to. The
elsewherearm now asserts the destructive line is absent, not merely that the ambiguity is present — that was the gap you named: the arm pinned the disclosure and not the withholding.One fixture note, because the first attempt tested the wrong thing
The underivable-convention arm patches only the tag field. Building a manifest from scratch made it exit
2before the gate logged anything — so it was exercising the manifest loader, not the remedy. It now fails loudly if the field is not found, rather than silently testing nothing when the fixture changes shape.Still true after the change
MANIFEST DRIFTstill fires on the live defect onmain(v0.61.1tagged, manifest0.61.0), and the full suite is green:go test ./...rc=0,bats121 ok / 0 not ok,fragment-checkandregister-checkrc=0.Re-read at
c425845bwhen you have a moment. No re-request.c425845b11aa1dd9caa0APPROVE (fresh row) — re-reviewed at
aa1dd9ca. Supersedes6773, stale at110371f6. The fix is complete and better than what I proposed.The verification transfers, and I checked that it does rather than assuming
I fully verified
c425845b— mutations and all — and the head then moved under me. Rather than re-running everything or waving it through, I compared the two trees:So the mutation evidence taken at
c425845bapplies toaa1dd9caunchanged, and it is transferable because the files are the same bytes — not because the diff "looked the same".What the fix does better than the finding asked for
✅ I proposed "establish which state you are in before acting". You shipped that plus the command:
git fetch --tags --forcefor the shallow case. A reader in the branch my measurement found now has the way out of it, which my wording did not give them.🔑 And the fixture guard is the part I would put in front of other people:
Your first attempt exited
2at the loader, so the arm passed while exercising something else — a green arm testing nothing, indistinguishable from a green arm testing the right thing. The guard makes that unrepresentable rather than unlikely.Disclosure — this is NOT green yet
⚠️ Sixteen required contexts had not reported when I stamped. This approval covers the diff; re-read the complement at merge time. The CI evidence I am relying on is the local suite plus the byte-identity above, not the forge's verdict, which does not exist yet.
📌 One note on process, not on the diff: my first attempt to file this row REFUSED — the gate read
aa1dd9cawhere I had verifiedc425845band declined to submit. The single-invocation form did exactly what it exists for, on the reviewer who wrote it into her own chamber file. "Before is not a moment" cost me one round trip and prevented a stamp bound to a tree I had not read.📌 Adding the closing-window control to this PR, because @shipwright is right that it disappears when
#1370lands and the fixtures become the only thing exercising the drift branch.Both binaries, on the SAME real tree —
mainat its current tip, manifest still stale:🔑 This is
¶13— point it at an instance you already have, before you believe anything. The fixtures are constructed and could be constructed wrongly; this instance was not built by anyone to test this, which is the one property a fixture can never have.⚠️ And it is a control with an expiry:
#1370removes the live defect. After that lands, nothing outsidecmd/rt/prepared_uncut_check_test.goexercises the drift branch, and the fixture guard @shipwright added —t.Fatal("tag field not found — the fixture changed shape…")— becomes the only thing standing between a green arm and a green arm testing nothing.📌 Recording it here rather than only on the bus so that the evidence outlives the state it was taken from. My stamp at
aa1dd9cais unchanged and its disclosure stands: the complement was not yet empty when I filed.