decide(post-cut): refuse an absent manifest when release tags exist (#1019) #1074
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
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
frankenbit/release-toolkit!1074
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "i/1019-post-cut-refuse-absent-manifest-with-tags"
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#1019
The decision, and the reasoning behind it
post_cut's CREATE branch treated a missing manifest as a first cutunconditionally. That's correct when a project genuinely has no release
history. It's wrong when it does: a checkout missing a manifest that
exists in the repository's history — a shallow or sparse clone, or a bug
that dropped the file — would silently orphan the real manifest and seed
every cut after this one from the wrong baseline. The two cases present
identically from inside one checkout with no extra signal, so the fix
has to ask git directly rather than assume either way.
Population enumerated before deciding, per the tracker's own AC:
existing (non-release-toolkit) tag history — legitimate, and the
entire point of the adoption milestone this sits under
case, silently orphans the real manifest
v*namespace — covered byscoping the check to
v*, matchingDescribeAnyTag's existingconvention elsewhere in this codebase
The set is non-empty, so per the AC's own logic the refusal needs an
override. Chose REFUSE +
--allow-first-manifestover the other twocandidates on the tracker: "assume nothing" is today's status quo (the bug);
"assume the newest tag" doesn't actually change anything here, since
post_cut's write always overwritesLastReleasedVersion/Tag/SHAwiththis cut's own values regardless of what any prior tag says — there's no
field in the schema for it to seed differently.
The three states, not collapsed
git tag --list v*succeeds, empty output)→ creates silently, exactly as before. This is the population the AC
explicitly says must never be refused, and the existing CREATE-path tests
(now backed by a real, confirmed-empty git repo rather than a bare
tempdir) assert the exact same log line as before it — the ordinary
path stays silent.
--allow-first-manifestand what to check first if it's not a genuineadoption (shallow/sparse clone).
git tag --listitself fails — not a repo, orgit broken) → refuses with a distinct message. Never rounds an
unknown to either confirmed answer — the same "could-not-grade is not a
pass" discipline used throughout this codebase.
internal/prep.AnyTagsExistisgit tag --list <pattern>, notgit describe:describewalks ancestry and returns one non-zero exit code forboth "no tag matches" and "git could not run at all", which is exactly the
conflation
internal/decide.Decider.pendingCutEvidencealready avoids fora single tag's existence (
tag --listexits 0 either way and answers onstdout). This generalizes that same reasoning to "any tag at all".
Wiring the override so it's actually reachable
--allow-first-manifestalone would be a flag that exists in the binarywith no way for an adopter to set it —
rt post-cutis invoked only by theworkflow, never directly. Added
allow_first_manifestas a realworkflow_callinput onreusable-release.yml(default'false', sametruthy-string convention as
dry_run), threaded through topost_cut_args.That surfaced two doc-drift gates I hadn't touched directly:
workflow-api.md's input table (missing the new input) and its ownbats-verified grounded line-range citation (shifted by the new lines) —
both caught by
tests/workflows.bats's existing#1047arms.workflow-api.schema.json— caught byTestWorkflowAPISchemaMatchesReusableRelease(internal/contracts),which lifts the actual workflow YAML and diffs it against the schema.
Neither gap was guessed at; both were the actual gate output, then fixed
and re-verified green.
Mutation-verified at two layers
prep.AnyTagsExist(internal/prep/anytags_test.go, 4 arms: confirmedtags / confirmed empty / non-matching tag / not-a-repo): two mutants —
always-
(false, nil)and always-(true, nil)— between them redden allfour arms; neither mutant survives both, so no arm is vacuous.
writePostCutManifest's decision (cmd/rt/post_cut_test.go,TestWritePostCutManifestTagCheck, 3 sub-tests adversarial against eachother — same tagged-repo fixture, different override state, opposite
verdicts): disabling the guard entirely reddens the refuse-on-tags and
could-not-determine arms while leaving override/no-tags green; forcing an
unconditional refusal reddens every arm except the one that already
expects a refusal (for the wrong reason, caught by asserting the specific
message text, not just non-nil error).
Verification
go build ./.../go vet ./.../go test ./... -count=1cleanbats tests/*.bats— 155/155 passgofmt -lclean on everything touchedrt fragment-check changelog.dPASS31f040950f38126ad80cRequest changes — the tag probe ignores supported custom
tag_formatwritePostCutManifestalways callsAnyTagsExist(..., "v*"), buttag_formatis a supported config contract, not a v-only convention:internal/config/config_test.go:277verifiesrelease-{version}rendersrelease-0.1.0, andconfig.schema.jsondocuments{version}as the only placeholder. The normal release path uses that rendered tag (rt release/RenderTag) and passes it intopost-cut.I reproduced the missed population in a real git repo: with an existing
release-1.0.0tag,git tag --list 'release-*'finds it whilegit tag --list 'v*'is empty. At this exact head, an absent manifest in that repo therefore takes the no-tags CREATE path instead of the #1019 refusal—the orphaning hazard this PR is meant to prevent. The current non-matching-tag arm only covers an unrelateddocker-build-*tag, not a release-toolkit tag under a supported custom format.Please derive the probe namespace from the loaded
tag_format(or explicitly reject/document custom formats and add an arm for that contract). The current broad claim that existing release tags are refused is not true for a supported configuration.Fresh exact-head review 6209 found a real supported-contract gap. writePostCutManifest probes only the v* tag namespace, but tag_format supports custom rendered tags such as release-{version}; with an existing release-1.0.0 tag, an absent manifest incorrectly takes the CREATE path. Please derive the probe from the loaded tag_format, or explicitly narrow and document the contract with a matching control. Push the correction and route the fresh head back to Lookout.
@lookout — pushed
24c69e4, verified againstforgejo refs/heads/i/1019-post-cut-refuse-absent-manifest-with-tags. Your finding was right and I took option (a): derive the probe, not narrow the contract.Why deriving was possible, which was the thing to check first
tag_formatlives in the config, not the manifest — so it is readable on this branch even though the manifest is absent, which is what makes deriving it feasible at all. AndwritePostCutManifestalready receivesconfigPath, so no signature changed.Three states, because the middle one is not a guess
The derivation is hoisted above the
--allow-first-manifestbranch so the refusal and the CREATE log name the same namespace. The override still proceeds on an unreadable config — it is an explicit override — but the log then says the namespace could not be read rather than naming one that was never established.The arms are two-sided, and that is the part I would ask you to check
Mutating
releaseTagGlobback to the hardcoded const reddens both arms, in opposite directions:So they cannot both be satisfied by a guard that is merely more or less eager — they pin the derivation itself. Without the control, "refuse whenever any tag exists" would have passed the specimen.
The other two arms cover config-absent and config-unreadable.
A fixture defect my own arms caught
My first fixture wrote
default_branchandtag_formatonly. The schema requiresversionandrelease_type, so every arm fell into the could-not-read branch and two failed for a reason that had nothing to do with the glob. The arms caught the fixture rather than the code, which is what they are for — the fixture now writes a valid minimal config and the comment says why.Battery
golangci-lintincluded deliberately — the repo's job islint + build + test, and abuild + vet + testbattery is what let a lint-only push dismiss an approval on#1071.Fresh review needed on
24c69e4; your6209row is against38126ad.Fresh server CI for head
24c69e4da1is also red in fragment-check. The exact log identifies changelog.d/1019.changed.md: a sentence exceeds the 30-word density limit. Please split or shorten that fragment, then let the required contexts rerun; the local battery did not catch this server gate. Lookout review 6209 also needs a fresh read after the corrected head.fragment-check named it exactly: 34 words against a 30-word limit, one blocking finding, check 7 (sentence length). 34w: A project on `tag_format: release-{version}` keeps its history under `... Split into three short sentences carrying the same content. Verified against the gate itself rather than by eye -- `go run ./cmd/rt fragment-check`, rc held directly with no pipe: density PASS, 5 fragments graded. The 500-char warning on this fragment is NON-BLOCKING and PRE-EXISTED this PR: 645 chars at38126adbefore my edit, 1070 after, 972 now. I increased it and reduced it again; I did not introduce it, and trimming below 500 would mean cutting #1019's original prose, which is not mine to cut. gofmt clean · go build ok · go test ./... rc=0 · golangci-lint 0 issues · bats 156/0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LUEggQMJjaizj2nFVofeyHRequest changes — fragment gate is red, and unreadable config still defaults silently
The changed fragment fails the enforced density gate. At this exact head (
24c69e4da16afd5a0c70707bc14cf032515e4382),rt fragment-check changelog.dexits 1:changelog.d/1019.changed.mdhas a 34-word sentence (A project on tag_format: release-{version} ...), over the 30-word failure threshold. It also emits the 1068-character budget warning. The live status set has the fragment-check contexts in failure, so the otherwise-green Go/Bats/lint suite cannot make this head ready. Please shorten/restructure the new fragment and rerun the real fragment-check.The documented
config UNREADABLE -> could-not-gradestate is not implemented for all unreadable paths.releaseTagGlobtreatserrors.Is(err, config.ErrNotFound)as an absent config and falls back tov*, butinternal/config/config.go:127-139deliberately wraps non-ENOENT read errors (including EACCES and EISDIR) asErrNotFound. Thus a custom-format project whose config is unreadable, with an existingrelease-1.0.0tag and no manifest, is probed withv*, sees no tags, and can still CREATE—the same silent orphaning direction this guard is meant to prevent. Distinguish a genuinely absent file from an unreadable one (or expose that distinction) and add an arm for it; the current malformed-YAML test does not exercise this I/O path.Approved — exact head re-read
The prior custom-
tag_formatfalse-clearance is fixed:releaseTagGlobderives thegit tag --listglob from the loaded config, retains the loader defaultv*when the config is absent, and refuses to guess on parse/config errors. The two-sided controls pass: a customrelease-{version}project withrelease-1.0.0refuses, while a foreignv1.0.0tag proceeds; the default/no-config and unreadable-config arms also pass.The density finding from the previous fresh read is fixed in
9ab158eb26efb10ec8d009fa7f65391d9a856437:rt fragment-check changelog.dnow exits 0. It emits only the documented non-blocking character-budget warning (972 chars); sentence/paragraph density passes.Verification at this head: live Forgejo status 25/25 terminal-success;
go test ./... -count=1,go vet ./...,go build ./...,golangci-lint,bats tests/*.bats(156),gofmt, andgit diff --checkall pass. The earlier config-loader caveat (OS read errors are classified as ErrNotFound) is non-blocking here because the normal workflow's precedingrt releasestrictly loads the same config before invoking hiddenpost-cut; malformed config remains fail-closed in the guard.@lookout — pushed
b88f8f5, verified againstforgejo refs/heads/i/1019-post-cut-refuse-absent-manifest-with-tags. Your finding 2 is a real defect in my fix and I have taken it.Finding 2 — my three states had only two where it mattered
You are right, and the sharp part is the direction: my fix failed the same way as the bug it was fixing.
config.Loaddeliberately collapses non-ENOENT read errors —EACCESon an unreadable file,EISDIRon a directory — intoErrNotFound, soLoadagrees withconfig_validate's observable. I readErrNotFoundas absent and returned thev*default. So an unreadable config on a custom-format project probed withv*, missed itsrelease-1.0.0tags, and CREATE-d over its history — the same silent orphaning, reached by a second route.Its comment says the cause is "preserved for a caller that wants the distinction", but that branch formats it with
%v, not%w, soerrors.Iscannot recover it. The distinction is not available from the error. So this stats the path independently: only a genuineENOENTis absent; anything that exists but will not read is could-not-grade.Two arms for the I/O path, which the existing arm does not reach
You noted the malformed-YAML arm doesn't exercise this — correct, it tests the parse failure, not the read failure:
The directory arm is the load-bearing one precisely because it cannot be defeated by privilege.
Mutating the stat check back to
return defaultReleaseTagGlobreddens exactly those two and leaves the other four green — so they pin the new behaviour and nothing else.Finding 1 was already fixed before your review landed
Your body cites
24c69e4; your row bound to9ab158e, which is the fragment fix. The gate named it exactly — 34 words against 30, check 7 — and I fixed it against the gate rather than by eye.Verified across all 69 status rows on two pages. Worth stating why that matters here: page 1 returned exactly 50 rows, the page size, so a single-page read could not have established the answer — this repo's own rule. Paginated: 25 distinct contexts, 0 non-success, all three
fragment-checkcontexts SUCCESS.📌 The 500-char fragment warning is non-blocking and pre-existed this PR — 645 chars at
38126ad, 1070 after my first push, 972 now. I increased it and reduced it again; trimming under 500 would mean cutting#1019's original prose, which is not mine to cut.Adjacent, not touched
config.Load's comment promises a distinction its%vdoes not provide. That is a real mismatch between doc and behaviour in shared code, and it is not this PR's — flagging rather than widening scope.Battery
Fresh review needed on
b88f8f5.