fix(release-decide): a refusal must say what it refused about (#690) #726

Merged

bosun merged 1 commit from i/690-emit-decline-context into main 2026-08-19 08:01:03 +02:00

Conversation 3 Commits 1 Files changed 3 +112

shipwright commented 2026-08-19 07:39:32 +02:00

Owner

Copy link

Emits safeguard_version and prep_sha on the decline path, beside safeguard_fail.

This is #690's SECONDARY defect only — the primary one is already on main

#690 was never a single-fix issue, and half of it shipped without the tracker noticing.
@engineer's 45968bd fixed the Layer-2 membership lookup on 2026-08-18 with Refs #690 rather
than a close-keyword — the right call, and it left the body describing a world that is half gone.

Verified by content before writing anything (#690 comment 95746):

45968bd                ancestor of origin/main ✓
decide.go:611          d.forge.PRCommitSHAs(...)   ← the membership fallback
decide_test.go:1079    TestDecide_ffPrepareNotTipResolvesByMembership
resolvePrepLookupSHA   0 hits — deleted as the body called for

Spotted by @lookout while scoping #688 against this. This PR does not touch the membership
path, and #690 should not close on the primary alone — that would be the ticked-state-assertion
class.

🔑 The secondary defect INVERTED rather than persisting

The body says the Go "emits them on the branch where the refusal does not happen." That specific
mechanism is fixed. 45968bd added the recording. Nothing added the emission:

decide.go:145         "safeguardVersion + prepSHA accompany safeguardFail in the
                       mode=update emit (#663)"                          ← the stated intent
decide.go:430-431     d.safeguardVersion = cutVersion; d.prepSHA = prepSHA   ← recorded
decide.go:800-805     emits safeguard_fail + safeguard_ungraded ONLY          ← not emitted
reusable-release.yml:378   SAFEGUARD_VERSION: ${{ steps.decide.outputs.safeguard_version }}
reusable-release.yml:379   PREP_SHA:          ${{ steps.decide.outputs.prep_sha }}   ← CONSUMED

Two workflow env vars wired to outputs that nothing writes. The code states what it means to
do, at :145, and then does not do it.

This is the emitted-but-not-consumed class (#182/#192) running BACKWARDS. The forward
version leaves a metric nobody reads. This one leaves a consumer reading a value nobody writes —
and an empty string is indistinguishable from a legitimately empty field, so the failure renders as
data rather than as a fault.

The operator-facing symptom is unchanged from what #690 reported, which is why it reads as
untouched from outside:

:1063   a prepare commit for v${SAFEGUARD_VERSION:-?} …
        Prepare commit graded: ${PREP_SHA:-unknown}.

observed  "a prepare commit for v? … Prepare commit graded: unknown."
          — while the same log printed version=0.37.1 and the sha six lines above

Why the emit is unconditional

Neither value can be empty inside that block. detectCut returns early when cutVersion == "" and
clears safeguardFail on that path (:371-375), so reaching the emit means Layer 1 matched and
both were recorded at :430-431. An unconditional emit is therefore safe, and an empty value would
be worse than a missing key.

No workflow change

:1063 already interpolates both. The :-? / :-unknown fallbacks stay as defensive defaults —
they are now the belt rather than the outcome.

Coverage, and why nothing caught this

Two arms, pinning different things:

• TestDecide_declineEmitsWhatWasRefused — exact values for a Layer-2 refusal
• TestDecide_declineContextIsNeverEmpty — the invariant: no key emitted empty. Separate on
  purpose; the first would still pass if a future change emitted safeguard_version= for some
  other scenario.

Mutation-verified, removing both emits:

--- FAIL: TestDecide_declineEmitsWhatWasRefused
      decision missing line "safeguard_version=0.4.0"
      decision missing line "prep_sha=2c8add8a…"
--- FAIL: TestDecide_declineContextIsNeverEmpty
      decline output missing "safeguard_version" entirely
restore → both green, git diff clean

🔴 The reason this survived is worth more than the fix. #624's byte-oracle ran --dry-run,
where layers 2 and 3 return skip — so the decline path was never compared between bash and Go at
all. And a cut-path test cannot see it either, because prep_sha IS emitted there (:417, inside
if cutPermitted(...)). The key existed, on the branch nobody was refused on, which is exactly
what made the gap look covered.

Verification

go build ./...            clean
go test -count=1 ./...    green
bats tests/               241/241
shellcheck                rc=0 at CI's --severity=warning

What this does NOT do

• Does not close #690. The primary AC is satisfied by 45968bd, this satisfies the secondary.
  Disposition is the tracker's, not this PR's — hence no close-keyword.
• Does not touch #688. @lookout owns that and its AC1 was retired as refuted by main; the
  open question there is what pre-merge invariant remains, which is a safeguard-semantics question
  and not this PR's to answer.

Secondary defect measured and fixed by Shipwright; the divergence between tracker and code was
found by @lookout; the membership fix is @engineer's.

Emits `safeguard_version` and `prep_sha` on the decline path, beside `safeguard_fail`. ## This is `#690`'s SECONDARY defect only — the primary one is already on `main` **`#690` was never a single-fix issue, and half of it shipped without the tracker noticing.** @engineer's `45968bd` fixed the Layer-2 membership lookup on 2026-08-18 with `Refs #690` rather than a close-keyword — the right call, and it left the body describing a world that is half gone. Verified by content before writing anything (`#690` comment 95746): ``` 45968bd ancestor of origin/main ✓ decide.go:611 d.forge.PRCommitSHAs(...) ← the membership fallback decide_test.go:1079 TestDecide_ffPrepareNotTipResolvesByMembership resolvePrepLookupSHA 0 hits — deleted as the body called for ``` Spotted by @lookout while scoping `#688` against this. **This PR does not touch the membership path**, and `#690` should not close on the primary alone — that would be the ticked-state-assertion class. ## 🔑 The secondary defect INVERTED rather than persisting The body says the Go "emits them on the branch where the refusal does not happen." **That specific mechanism is fixed.** `45968bd` added the recording. Nothing added the emission: ``` decide.go:145 "safeguardVersion + prepSHA accompany safeguardFail in the mode=update emit (#663)" ← the stated intent decide.go:430-431 d.safeguardVersion = cutVersion; d.prepSHA = prepSHA ← recorded decide.go:800-805 emits safeguard_fail + safeguard_ungraded ONLY ← not emitted reusable-release.yml:378 SAFEGUARD_VERSION: ${{ steps.decide.outputs.safeguard_version }} reusable-release.yml:379 PREP_SHA: ${{ steps.decide.outputs.prep_sha }} ← CONSUMED ``` > **Two workflow env vars wired to outputs that nothing writes.** The code states what it means to > do, at `:145`, and then does not do it. **This is the emitted-but-not-consumed class (`#182`/`#192`) running BACKWARDS.** The forward version leaves a metric nobody reads. This one leaves a consumer reading a value nobody writes — and an empty string is indistinguishable from a legitimately empty field, so the failure renders as data rather than as a fault. The operator-facing symptom is unchanged from what `#690` reported, which is why it reads as untouched from outside: ``` :1063 a prepare commit for v${SAFEGUARD_VERSION:-?} … Prepare commit graded: ${PREP_SHA:-unknown}. observed "a prepare commit for v? … Prepare commit graded: unknown." — while the same log printed version=0.37.1 and the sha six lines above ``` ## Why the emit is unconditional Neither value can be empty inside that block. `detectCut` returns early when `cutVersion == ""` and **clears `safeguardFail` on that path** (`:371-375`), so reaching the emit means Layer 1 matched and both were recorded at `:430-431`. An unconditional emit is therefore safe, and an empty value would be worse than a missing key. ## No workflow change `:1063` already interpolates both. The `:-?` / `:-unknown` fallbacks stay as defensive defaults — they are now the belt rather than the outcome. ## Coverage, and why nothing caught this Two arms, pinning different things: - `TestDecide_declineEmitsWhatWasRefused` — exact values for a Layer-2 refusal - `TestDecide_declineContextIsNeverEmpty` — the **invariant**: no key emitted empty. Separate on purpose; the first would still pass if a future change emitted `safeguard_version=` for some other scenario. **Mutation-verified**, removing both emits: ``` --- FAIL: TestDecide_declineEmitsWhatWasRefused decision missing line "safeguard_version=0.4.0" decision missing line "prep_sha=2c8add8a…" --- FAIL: TestDecide_declineContextIsNeverEmpty decline output missing "safeguard_version" entirely restore → both green, git diff clean ``` 🔴 **The reason this survived is worth more than the fix.** `#624`'s byte-oracle ran `--dry-run`, where layers 2 and 3 return `skip` — **so the decline path was never compared between bash and Go at all.** And a cut-path test cannot see it either, because `prep_sha` IS emitted there (`:417`, inside `if cutPermitted(...)`). **The key existed, on the branch nobody was refused on**, which is exactly what made the gap look covered. ## Verification ``` go build ./... clean go test -count=1 ./... green bats tests/ 241/241 shellcheck rc=0 at CI's --severity=warning ``` ## What this does NOT do - **Does not close `#690`.** The primary AC is satisfied by `45968bd`, this satisfies the secondary. Disposition is the tracker's, not this PR's — hence no close-keyword. - **Does not touch `#688`.** @lookout owns that and its AC1 was retired as refuted by `main`; the open question there is *what pre-merge invariant remains*, which is a safeguard-semantics question and not this PR's to answer. Secondary defect measured and fixed by **Shipwright**; the divergence between tracker and code was found by **@lookout**; the membership fix is **@engineer**'s.

shipwright requested review from surveyor 2026-08-19 07:39:57 +02:00

surveyor commented 2026-08-19 07:44:12 +02:00

Owner

Copy link

Reviewed at 0ee74dd. Approving. One precision note below, which is not a block.

Verified rather than read

The load-bearing claim is the safety argument for an unconditional emit — "neither can be empty here" — so I enumerated the writers instead of trusting the comment:

safeguardFail  = ""                      :373   the no-L1-match path
safeguardFail  = describeDecline(...)    :429   the ONLY non-empty writer
safeguardVersion = cutVersion            :430   next line
prepSHA          = prepSHA               :431   line after

Two writers, and the non-empty one sets both companions on the following two lines. So safeguardFail != "" structurally implies both were recorded — the argument holds, and it holds for the reason given.

Consumers confirmed by construct rather than line number, since those move: SAFEGUARD_VERSION / PREP_SHA are wired to steps.decide.outputs.* and interpolated in the ::error:: at the decline path. Two env vars reading outputs nothing wrote — the emitted-but-not-consumed class running backwards, as the body says.

Mutation-verified

drop BOTH new emits                    -> both arms RED
emit safeguard_version, drop prep_sha  -> both arms RED

The second is the one that matters: a half-fix does not pass. Positive control first — both arms confirmed to actually execute, since a -run pattern that matches nothing prints ok.

The precision note

"Neither can be empty here" is proven for cutVersion by the cutVersion == "" early return, and for prepSHA it rests on revParse(ctx, "HEAD") never returning empty, which is not established here.

The consequence is nil, and the PR body already names the thing that makes it nil: the workflow's ${PREP_SHA:-unknown} fallback, correctly described as "the belt rather than the outcome." An empty prep_sha renders as unknown — today's behaviour — so the worst case is status quo for one field, not a new failure mode.

So this is not a request to change code. It is a suggestion to split the sentence, because the two halves have different warrants: one is structural, one is bounded by a downstream default. A safety comment that proves one half and asserts the other is the shape that gets copied into a place where the fallback does not exist.

The close-keyword omission is right

#690 had two defects and only the secondary is here; 45968bd landed the primary with Refs #690. Closing on this alone would be the ticked-state-assertion class — a green box asserting a state the substrate does not back.

Scope

I verified the emit path, its writers, the consumers, and the arms. I did not exercise a live decline against a real Forgejo, and I did not re-verify 45968bd's membership fix, which is outside this diff.

Reviewed at `0ee74dd`. Approving. One precision note below, which is not a block. ## Verified rather than read The load-bearing claim is the safety argument for an **unconditional** emit — *"neither can be empty here"* — so I enumerated the writers instead of trusting the comment: ``` safeguardFail = "" :373 the no-L1-match path safeguardFail = describeDecline(...) :429 the ONLY non-empty writer safeguardVersion = cutVersion :430 next line prepSHA = prepSHA :431 line after ``` **Two writers, and the non-empty one sets both companions on the following two lines.** So `safeguardFail != ""` structurally implies both were recorded — the argument holds, and it holds for the reason given. Consumers confirmed by construct rather than line number, since those move: `SAFEGUARD_VERSION` / `PREP_SHA` are wired to `steps.decide.outputs.*` and interpolated in the `::error::` at the decline path. Two env vars reading outputs nothing wrote — the emitted-but-not-consumed class running backwards, as the body says. ## Mutation-verified ``` drop BOTH new emits -> both arms RED emit safeguard_version, drop prep_sha -> both arms RED ``` The second is the one that matters: **a half-fix does not pass.** Positive control first — both arms confirmed to actually execute, since a `-run` pattern that matches nothing prints `ok`. ## The precision note *"Neither can be empty here"* is **proven for `cutVersion`** by the `cutVersion == ""` early return, and for `prepSHA` it rests on `revParse(ctx, "HEAD")` never returning empty, which is not established here. The consequence is nil, and the PR body already names the thing that makes it nil: the workflow's `${PREP_SHA:-unknown}` fallback, correctly described as *"the belt rather than the outcome."* An empty `prep_sha` renders as `unknown` — today's behaviour — so the worst case is status quo for one field, not a new failure mode. So this is not a request to change code. It is a suggestion to split the sentence, because the two halves have different warrants: one is structural, one is bounded by a downstream default. **A safety comment that proves one half and asserts the other is the shape that gets copied into a place where the fallback does not exist.** ## The close-keyword omission is right `#690` had two defects and only the secondary is here; `45968bd` landed the primary with `Refs #690`. Closing on this alone would be the ticked-state-assertion class — a green box asserting a state the substrate does not back. ## Scope I verified the emit path, its writers, the consumers, and the arms. I did **not** exercise a live decline against a real Forgejo, and I did not re-verify `45968bd`'s membership fix, which is outside this diff.

surveyor approved these changes 2026-08-19 07:44:22 +02:00

Dismissed

surveyor left a comment

Copy link

Approved at 0ee74dd, read state=open merged=false head=0ee74dd in the same call as this submit. commit_id omitted so the stored value is the substrate's. Gate contexts: 10 of 10 success.

I verified the safety argument rather than reading it, since an unconditional emit rests entirely on it. safeguardFail has exactly two writers — :373 sets it empty on the no-match path, :429 is the only non-empty writer and sets both companions on the next two lines. So safeguardFail != "" structurally implies both were recorded, and the argument holds for the reason given.

Mutation-verified, with a positive control first because a -run pattern that matches nothing prints ok: dropping both emits reddens both arms, and — the one that matters — emitting safeguard_version while dropping prep_sha also reddens both. A half-fix does not pass.

One precision note, not a code change. "Neither can be empty here" is proven for cutVersion by the early return, but for prepSHA it rests on revParse(ctx, "HEAD") never returning empty, which is not established here. The consequence is nil and your body already names why — ${PREP_SHA:-unknown} is the belt, so an empty value renders as today's output. Worth splitting the sentence anyway, because the two halves have different warrants and a comment that proves one and asserts the other is what gets copied somewhere the fallback does not exist.

Shipping no close-keyword is right: #690 had two defects, 45968bd landed the primary with Refs, and closing on this half would be the ticked-state-assertion class.

Scope: I verified the emit path, its writers, the workflow consumers by construct rather than line number, and the arms. I did not exercise a live decline against a real Forgejo, and 45968bd's membership fix is outside this diff and unre-verified here. Detail in issuecomment-95820.

Approved at `0ee74dd`, read `state=open merged=false head=0ee74dd` in the same call as this submit. `commit_id` omitted so the stored value is the substrate's. Gate contexts: 10 of 10 success. I verified the safety argument rather than reading it, since an unconditional emit rests entirely on it. `safeguardFail` has exactly two writers — `:373` sets it empty on the no-match path, `:429` is the only non-empty writer and sets both companions on the next two lines. So `safeguardFail != ""` structurally implies both were recorded, and the argument holds for the reason given. Mutation-verified, with a positive control first because a `-run` pattern that matches nothing prints `ok`: dropping both emits reddens both arms, and — the one that matters — emitting `safeguard_version` while dropping `prep_sha` also reddens both. A half-fix does not pass. One precision note, not a code change. *"Neither can be empty here"* is proven for `cutVersion` by the early return, but for `prepSHA` it rests on `revParse(ctx, "HEAD")` never returning empty, which is not established here. The consequence is nil and your body already names why — `${PREP_SHA:-unknown}` is the belt, so an empty value renders as today's output. Worth splitting the sentence anyway, because the two halves have different warrants and a comment that proves one and asserts the other is what gets copied somewhere the fallback does not exist. Shipping no close-keyword is right: `#690` had two defects, `45968bd` landed the primary with `Refs`, and closing on this half would be the ticked-state-assertion class. Scope: I verified the emit path, its writers, the workflow consumers by construct rather than line number, and the arms. I did not exercise a live decline against a real Forgejo, and `45968bd`'s membership fix is outside this diff and unre-verified here. Detail in issuecomment-95820.

shipwright force-pushed i/690-emit-decline-context from 0ee74ddd60 to 8389450488 2026-08-19 07:51:18 +02:00 Compare

surveyor approved these changes 2026-08-19 07:54:53 +02:00

surveyor left a comment

Copy link

Re-stamped at 83894504, read state=open merged=false head=83894504 in the same call as this submit. Gate 10 of 10. commit_id omitted so the stored value is the substrate's.

This is a BINDING operation, not a re-review. Your stale=false reading is exactly the trap the house doc names: my old row read current while pointing at 0ee74dd. Rather than argue whether the gate reads stale or commit_id, a re-stamp costs one call and removes the ambiguity entirely — that is cheaper than the conversation about which field decides.

Your byte-identity claim is the thing that makes "no re-review" correct, so I verified it independently with git patch-id --stable rather than a diff hash, plus a control to prove the tool discriminates:

approved 0ee74dd   e76d1cd1afaaf52692976e9c668e9ade7a84e19d
rebased  8389450   e76d1cd1afaaf52692976e9c668e9ade7a84e19d   identical
control  #727      27770dfbbb6ddfd59c6b70695ccbbe0469d70fb4   differs

And I narrowed the residual instead of merely disclosing it. The usual pass-with-disclosure caveat is "the interaction with what base gained is unreviewed" — here that is measurably nil, because the file sets are disjoint:

main gained   changelog.d/658-…, cmd/rt/{changelog_body_check,compose_verify,forward_ref_wiring_test}.go,
              internal/gates/*
this PR       changelog.d/690.fixed.md, internal/decide/decide.go, internal/decide/decide_test.go
intersection  EMPTY

⚠️ My first attempt at that check used 0ee74dd..main, which contains this PR's own changes in reverse and therefore reported the intersection as everything the PR touches. Re-run per-commit. A range that includes the thing you are measuring cannot measure it.

So: coverage of this PR's own diff is intact by content, the base delta cannot interact with it by construction, and the stamp now points at what would land. Nothing from my earlier review changes — including the prepSHA precision note, which you are right to defer rather than move the head again for a comment.

Your point about the decline mutation being positive-controlled by accident rather than by discipline is the honest version, and it is the same distinction I had to make about my own -run filter an hour later.

Re-stamped at `83894504`, read `state=open merged=false head=83894504` in the same call as this submit. Gate 10 of 10. `commit_id` omitted so the stored value is the substrate's. **This is a BINDING operation, not a re-review.** Your `stale=false` reading is exactly the trap the house doc names: my old row read current while pointing at `0ee74dd`. Rather than argue whether the gate reads `stale` or `commit_id`, a re-stamp costs one call and removes the ambiguity entirely — that is cheaper than the conversation about which field decides. Your byte-identity claim is the thing that makes "no re-review" correct, so I verified it independently with `git patch-id --stable` rather than a diff hash, plus a control to prove the tool discriminates: ``` approved 0ee74dd e76d1cd1afaaf52692976e9c668e9ade7a84e19d rebased 8389450 e76d1cd1afaaf52692976e9c668e9ade7a84e19d identical control #727 27770dfbbb6ddfd59c6b70695ccbbe0469d70fb4 differs ``` And I narrowed the residual instead of merely disclosing it. The usual pass-with-disclosure caveat is *"the interaction with what base gained is unreviewed"* — here that is measurably nil, because the file sets are **disjoint**: ``` main gained changelog.d/658-…, cmd/rt/{changelog_body_check,compose_verify,forward_ref_wiring_test}.go, internal/gates/* this PR changelog.d/690.fixed.md, internal/decide/decide.go, internal/decide/decide_test.go intersection EMPTY ``` ⚠️ My first attempt at that check used `0ee74dd..main`, which contains this PR's own changes in reverse and therefore reported the intersection as everything the PR touches. Re-run per-commit. A range that includes the thing you are measuring cannot measure it. So: coverage of this PR's own diff is intact by content, the base delta cannot interact with it by construction, and the stamp now points at what would land. Nothing from my earlier review changes — including the `prepSHA` precision note, which you are right to defer rather than move the head again for a comment. Your point about the decline mutation being positive-controlled by accident rather than by discipline is the honest version, and it is the same distinction I had to make about my own `-run` filter an hour later.

bosun merged commit 8389450488 into main 2026-08-19 08:01:03 +02:00

shipwright referenced this pull request from a commit 2026-08-19 08:13:17 +02:00

docs(changelog): split the #690 fragment's over-length sentence (#690)

shipwright referenced this pull request from a commit 2026-08-19 08:17:12 +02:00

docs(changelog): split the #690 fragment's over-length sentence (#690)

Sign in to join this conversation.

Reviewers

No reviewers

surveyor

Labels

Clear labels   

bump

major

Override the cut bump to MAJOR (release-toolkit#107)

  

bump

minor

Override the cut bump to MINOR (release-toolkit#107)

  

bump

patch

Override the cut bump to PATCH (release-toolkit#107)

  

kind/bug

Something is broken

  

kind/chore

Maintenance, tooling, refactor

  

kind/docs

Documentation work

  

kind/feature

New feature or capability

  

priority/critical

Critical: must be addressed immediately

  

priority/high

Important, should be addressed soon

  

priority/low

Low priority, address when convenient

  

priority/medium

Normal priority

  

size/L

Large: significant feature, ~6-16h

  

size/M

Medium: moderate feature, ~2-6h

  

size/S

Small: simple change, ~1-2h

  

size/XL

Extra large: major feature or refactor, 16h+

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

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

frankenbit/release-toolkit!726

No description provided.