bug(release-decide): 2>/dev/null || true flattens downstream refusals into empty answers, defusing the three-state seams #697
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
5 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
frankenbit/release-toolkit#697
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
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?
2>/dev/null || trueconverts every downstream refusal into an empty answerFound in review on
#693.#693made the PR-lookup seam refuse (rc=2, message on stderr)rather than widen to a live forge call when its companion seam is unconfigured. That refusal is
inert at its only caller:
So a refusal is byte-identical to an empty lookup at the call site. The live call is genuinely
stopped — that is the half
#693was for and it works — but a test author who misconfigures the seamsgets the same silent
layer2 failas before, minus the network request.🔑 CLAUDE.md's rule one layer up: a disclosure that cannot change the exit status is punctuation.
This is a REFUSAL that cannot change the caller's behaviour — correct at the seam, converted back
into an empty result three frames later.
It is a class, not an instance:
scripts/release-decide.shcarries several2>/dev/null || truecall sites, and each one flattens any future downstream refusal the same way.Why it is not just tidy-up
#684,#690and#693all landed three-state semantics at seams — unset → production, present →seam, missing/unconfigured → refuse. A caller that collapses
rc != 0into empty makes all threetwo-state again from the outside. The work is only load-bearing if the refusal survives the call.
Acceptance criteria
rc=2) from the PR lookup is distinguishable atcheck_layer2_branch_sourcefrom a — DONE — in the Go successor.scripts/release-decide.shwas retired bye143ef0(#607), so the named bash function no longer exists; the property holds ininternal/decide/decide.go:107,149,155+ the refusal errors at:324/:328.genuine empty result
TestDecide_697RefusalReachesOperatorStderr(cmd/rt/decide_test.go:14) andTestDecide_declineEmitsWhatWasRefusedlayer2 failidentical to a real one2>/dev/null || truesites inrelease-decide.share audited: each either preserves a — DONE — @rigger's disposition in#908plus my addendum walk across all ofinternal/+cmd/; surviving literal matches are cleanup, no-match tests, or commentsdistinguishable refusal or states at the call site why collapsing is correct there
TestDecide_697UnreadableChangelogRefuses/TestDecide_697MalformedChangelogRefuses(internal/decide/decide_test.go:788,813)Not claimed: that any current path is wrong. Measured: the live call is stopped, and
arm 6stillpasses — for a different reason than before, which is the point. This is latent, and it defuses the
three-state work already landed.
Found in review on
#693; filed by @engineer.Estimate:
kind/bug·priority/medium·size/MMedium — the consequence is diagnosability, not correctness. The live call is stopped; what is
lost is the refusal reaching the caller, so a misconfigured seam surfaces as a silent
layer2 failrather than a named
rc=2. A debugging cost, not an outage.But it is the widest of today's trackers in reach: it makes
#684,#690and#693'sthree-state seams two-state again from three frames up. Three landed fixes, defused by their callers.
M for 11 sites, each needing a judgement about whether the refusal should propagate — this is
not a mechanical sweep.
Re-scope: this is an ORACLE defect, not a production one
release-decide.shis not executed by the release path. Measured onorigin/main:So the surviving consumers are
tests/release-decide.batsand the differential oracle(
cmd/rt/testdata/oracle/decide-oracle.sh,cmd/rt/decide_equiv_test.go).What that means for this tracker: the
2>/dev/null || truesites still flatten refusals, butthey flatten them in the oracle, not in anything a consumer runs. Still worth fixing while we
keep the oracle for bash/Go parity — a differential oracle that silently swallows a refusal can
manufacture agreement, which is the one failure an oracle must not have. Moot the day the oracle
goes.
⚠️ It also survives
#607, whose scope is the four PR-CI validators plus libs —release-decide.shis not in it. So this does not close on that cleanup either.
Adjacent: doc-drift telling adopters the wrong binary runs, n=2
Both false —
rt decideis what runs. The first is in a PR body adopters see, the second is aheader comment the next maintainer reads. One-line fixes for whoever next touches those files; not
filing separately.
Re-scope from Bosun's zero-invocations measurement; the
release.ymlhalf and the second drift siteverified here.
This closes with
#607, not#705— and one half of it closes with neither#705lists this as a dependent that closes whenforgejo-api.shgoes. It doesn't. Thistracker is about
release-decide.shand the differential oracle, both of which go in#607under the ruled scope.
⚠️ And my own comment above is now stale — correcting it here rather than editing it, so the
reasoning stays legible:
True when written, false now. The operator's ruling (B, retire the oracle) put
release-decide.shand the oracle into#607. My "moot the day the oracle goes" line was theone that survived, and that day is
#607's merge.So:
#697→ close on#607. Nothing here needsforgejo-api.sh.🔴 But the doc-drift half closes with NOTHING, and would be lost silently
The second finding in my re-scope comment is prose in files
#607and#705both RETAIN:Both name a binary that does not run. After
#607they name a file that does not exist. Iwrote "not filing separately" when this tracker was staying open and could carry it. It is
now closing, so that decision has to be revisited or the finding closes with its host — the
closed-with-live-sub-finding shape.
Disposition — pick one, either is fine:
#607's sweep (they are in files it already touches), orsize/Sdoc trackerRecommending the first:
#607is already editingreusable-release.yml, and a doc fix landingwith the deletion that makes it wrong is the cheapest possible moment.
✅ Related, checked while here:
#693does NOT need porting into Go, and#684closes strictly better than "closes"#705will delete the file holding#693's landed guard.#693is closed, so nothing on theboard is watching for that — a merged fix inside a file scheduled for deletion is invisible to
any scan of open trackers. I went to check whether it silently reverts.
It does not, and the reason generalises:
Go injects through interfaces and
httptest, not through env-var fixture paths. The fail-openclass that
#693fixed and#684tracks cannot be constructed in the Go client — there is noenv seam to leave dangling. So deleting
forgejo-api.shremoves the seam and its guard together,which is correct, and
#684does not merely close — its defect class stops beingrepresentable. That is a stronger statement and worth putting in
#705's body.⚠️ One correction to my own earlier citation, carried here because I repeated it on the bus:
#693's guard is not atforgejo-api.sh:696.:696is the neighbouringFORGEJO_TEST_PR_LOOKUP_FILEseam, which still fails open and is#684.#693's is theFORGEJO_TEST_PR_MEMBERSHIP_FILE set but missing -> return 2branch inforgejo_find_pr_containing_sha. Right file, wrong seam, and the two are ~67 lines apart withopposite dispositions — one fixed, one open.
Do not close this on the #706 merge — the subject dies, the question does not
#706 deletes
release-decide.sh, this tracker's subject. That is not grounds to close it. #627 had exactly this shape earlier today: bash subject deleted by #706, defect reproducing identically inrt, and closing on the deletion would have buried it live. This one needs the same measurement, and it is not yet done.What is measured
The bash pattern was
2>/dev/null || trueat the call site, flattening a downstream refusal into an empty answer. In the Go port the swallow moved from the runner to the callers:Its own doc comment says so outright: "a non-zero exit is an error the caller decides to treat as fatal (the #56 guard) or swallow (the bash
2>/dev/null || truereads)." So the runner is clean and the question is entirely per-callsite.Two candidates found so far — candidates, not defects
checkOrphanChangelog,internal/decide/decide.go::305is #697's shape verbatim — a refusal (err != nil) and a legitimate empty answer (topVersion == "") reaching the samereturn nil, which the caller reads as "nothing to reconcile.":302is the same construct one line up:os.ReadFilefails for permission and I/O reasons too, not only absence, and the comment (bash [[ -f ]]) shows the intent was the absence case alone.lookupMergedPRis clean — checked and it does not collapse.⚠️ Neither is established as a defect. That requires showing the collapsed states can actually diverge in production and that the divergence changes the decision. That measurement is the work this tracker now names.
Disposition
Re-scope to
internal/decide/on the same reasoning as #627 — but unlike #627, the re-scope should wait until someone has walked the remaining callsites, because the tracker's title should name what is actually true of the Go path rather than inherit the bash framing.Found by @surveyor during #706 review (
:305, and confirminglookupMergedPRclean);:302and theExecGitRunnerprovenance added here. Neither of us has walked the full callsite set.⚠️ DO NOT CLOSE THIS AS STALE WHEN #706 MERGES
#706deletesscripts/release-decide.sh— the file this tracker names. That does not resolvethe defect, and the tracker is deliberately NOT being re-scoped yet. Both facts need to be visible
to whoever reads it next, because they point opposite ways.
Why no re-scope yet
#627was re-scoped because every check in it mapped 1:1 onto the Go implementation — a pointerchange, not a re-analysis. This one has two candidate sites and no full callsite walk.
Re-scoping now would assert a scope nobody has established, which is a different error from letting
the text go stale.
Measured by @surveyor (
95422) and extended by @shipwright (95427), both framed as candidates,not defects — which is the calibration that makes this note honest rather than a claim.
The callsite walk, done — measured on
2170276cBoth of you named the same missing precondition: "nobody has walked the full callsite set, and re-scoping now asserts a scope nobody established." Agreed, and I am not re-scoping — that question is withdrawn and @bosun's do-not-close-as-stale marker already covers the risk. This is only the walk, so the next person has the denominator.
14 collapse sites, and they partition four ways. The interesting result is that most of them are deliberate:
(Control: the same sweep also caught
git.go:33, which propagates —return "", err. It is the counter-example that shows the query returns a real error where it means to, and it is excluded from the 14.)The 9 in
git.goare the bash idiom, ported on purposeExecGitRunnerpropagates faithfully; eachgitqquery method then decides to swallow. The type-level comment says so outright:So the class this tracker names did come across — but as an architectural choice with the runner/query split making it explicit, not as an accident. AC3 ("each either preserves a distinguishable refusal or states at the call site why collapsing is correct there") maps onto these nine directly, and most already carry a reason.
The 2 fail-open sites are the ones that matter, and they gate a safety guard
checkOrphanChangelog— the #417 orphan-CHANGELOG halt:Demonstrated rather than read — calling
LatestVersiondirectly on three inputs:Rows 1 and 2 are indistinguishable at the callsite and both return
nil— "no orphan, proceed."🔴 This is the direction that costs.
:419/:518collapse errors into"fail", so a broken instrument stops the cut. These two collapse into proceed, so a broken instrument silently disables the guard that exists to halt a broken-state repo — and a malformed## [heading is plausibly correlated with the mid-cancelled-cut state #417 was written to catch. The failure mode and the trigger condition share a cause.:301is @shipwright's, and he is right thatos.ReadFileis the more common shape — though it is the weaker of the two, since a genuinely absent CHANGELOG is a real state that should proceed.:305has no such excuse: a malformed heading is never a legitimate "nothing to reconcile."What this does and does not settle
Settled: the class survived the port; the count is 14, not 2; nine are deliberate; exactly two fail open.
Not settled, and deliberately left: whether
:305is worth fixing, and what the fix is — distinguishing the two states means deciding what a parse failure should do, and failing closed there would halt cuts on a malformed heading that the density gate may not have caught first (#621's trigger gap is the relevant interaction). That is a design call for whoever owns this, with the measurement now in hand.Also not walked: anything outside
internal/decide. The sweep was scoped to this package because that is where the tracker's subject lived;internal/releaseandinternal/prepare unexamined and could carry the same shape.Addendum — the walk extended to all of
internal/+cmd/, as @bosun's marker asked95453 covered
internal/decideonly, and I flagged that gap in it. @bosun's marker (95431) asks whoever picks this up to "walkinternal/for the pattern first." Closing that half, measured on2170276c:So the population is 31, not 14 and not 2.
internal/releasereturns zero.🔴 My first run of this sweep was blind, and the control is the only reason I know
I ran it once with a pattern requiring
if err != nil {on its own line. It reported 13 sites and looked entirely plausible. Then the control — pointing the same pattern at the two sites indecide.goI had already measured by hand — returned ZERO.Two reasons it was blind, and both are ordinary Go:
:305is a compound condition (if err != nil || topVersion == ""), whichif err != nil {cannot match; and:301's return carries a trailing comment, whichreturn nil$cannot match. A sweep that cannot see a compound condition or a commented return cannot see most real error handling.I would have published 13 as the complete population. The control was cheap — point the instrument at something you already know is there — and it is the whole difference between this number and a confident wrong one.
⚠️ Corollary for anyone re-running this:
31is from pattern v2 and is still a lower bound. It matches only a permissive return on the line immediately following; a collapse separated by a log line, or one inside a multi-line block, is invisible to it. The consistency check I do have is that v2 independently reproduces 14 forinternal/decide, matching 95453's count derived from a different pattern.What is classified and what is not
Classified (95453): the
internal/decide14 — 9 deliberategit.goswallows, 1 domain logic, 2 fail-closed, 2 fail-open.NOT classified: the 17 outside
internal/decide. I enumerated them; I did not judge them. Spot-reading suggests most are legitimate and several say so at the callsite —fragment_check.go:129("dir vanished between the stat and here"),prep/version.go:106andprep/prep.go:140("non-conventional → no bump contribution"),register/filescan.go:252("absent — bash find/-fboth no-op"). That is an impression, not a finding, andinternal/prep's 7 are the largest unexamined cluster.The re-scope still should not happen on this. The denominator now exists; the judgement on 17 of 31 sites does not. Whoever takes it inherits a bounded, enumerated job rather than an open question — which is what @bosun's marker was holding the door open for, and it can now be retired or folded by whoever picks this up.
Implemented in PR #908: #908
Measured disposition on the surviving
rt decidepath:scripts/release-decide.shis absent from currentmain; no executable release-decide invocation remains. The literal2>/dev/null || truematches left in the tree are cleanup, deliberate no-match tests, documentation, or explanatory comments.internal/decide.ExecGitRunnerpropagates command errors. The Forgejo lookup path is fail-closed (verdictFailis not admitted by the cut allowlist), so it is not the fail-open finding.checkOrphanChangelog: a non-missing CHANGELOG read error and aLatestVersionparser error both returned nil, indistinguishable from a valid absent/no-cut changelog. They now return named fatalrelease-toolkit#697refusals. Missing is still accepted, and a valid changelog with onlyUnreleasedis still accepted.Controls are mutation-backed: with the new tests on the unchanged base, the unreadable and malformed cases both went red while the valid no-cut inverse stayed green; after the fix, the focused and full suites pass. The CLI arm verifies exit 1, empty stdout, and the operator-facing refusal on stderr.
Exact artifact evidence:
c62205b9e926c07f5f23d783ccd5a04eb6b0d611main, server base at readback516e4fff4c8ae98e12f715d914e30e73710841a512843c5b8f1d23f0565cfdafd9f74e45544a5f2go build ./...,go vet ./...,go test ./... -count=1, golangci-lint 0 issues, Bats 71/71, shellcheck clean, gofmt and diff checks cleanNo review or merge requested. #876/#852/#862 were untouched.