fix(config): #625 config_render_tag propagates tag-format resolution failure #638

Merged

bosun merged 1 commit from i/625-config-render-tag-propagate-failure into main 2026-08-01 00:33:34 +02:00

Conversation 2 Commits 1 Files changed 3 +54 -1

engineer commented 2026-08-01 00:29:18 +02:00

Owner

Copy link

Refs #625.

config_render_tag assigned fmt=$(config_get_tag_format "$path") without checking
the status. With no release-toolkit.yml the getter returns 1 and prints nothing, so
config_render_tag returned 0 with empty output — and a caller's
|| printf 'v%s' fallback cannot fire on a success.

The distinction the fix rests on

Only _config_get_field can tell these apart:

condition	behaviour
file missing	returns 1 without printing the default (config.sh:60)
field missing	prints the v{version} default, returns 0

Propagating the getter's status keeps them distinct. Testing for empty output would
conflate them.

Why the fix is at the wrapper and not the shared helper

⚠️ On the tag_format path the two shapes are behaviourally identical — measured,
all four arms, because that wrapper's default is non-empty so empty-with-rc-0 is
unreachable there. No test can separate them, and I originally claimed otherwise;
that claim is retracted.

They diverge one layer down. config_get_schema_version passes an empty default,
so empty-with-rc-0 is its legitimate result — pinned today by
tests/config.bats:53 ("empty if missing", asserting status -eq 0 and empty
output). A helper-level "return 1 on empty" would redden it.

getter	empty default	correct rule
config_get_schema_version	meaningful	propagate status
config_get_release_type	invalid	tests for empty, by design (:87)
config_get_tag_format	non-empty default	the two coincide

Three contracts on one helper — which is why the fix belongs at the wrapper.

Blast radius: exactly two callsites

Six callers of config_render_tag. Three (manifest-check:208, release-prep:320,
draft-release:165) sit behind a config_validate gate that exits on a missing
config, so they are unreachable in the failing condition — verified that all three
gates are top-level and precede their render, and that config_validate returns 1 on
a missing file.

release-decide.sh is the only script with zero config_validate calls (config is
best-effort there by design), which is why #625 surfaces in decide and nowhere else.
Its two callsites already carry || printf 'v%s' fallbacks that were dead; this
resurrects them.

No behaviour change for any consumer. Establishing that required a positive control
on set -euo pipefail abort semantics, since a failing command substitution in an
assignment does abort under it.

The Go side needs no change

internal/decide/decide.go:721 already renders the v-prefix fallback unconditionally,
and its comment says it ports config_render_tag's || printf 'v%s'. This is bash
converging on Go, not a one-sided change.

Tests, and why this survived 842 arms

The existing config_render_tag arms all write a config first, so none could exercise
the no-config path — the fixture guaranteed the failing condition was unreachable.
Same reason the #622 nine-case differential missed it: every probe wrote a config.

The new no-config arm asserts its own precondition ([ ! -f "$CONFIG" ]). setup()
runs per-test and writes no file today; if that ever changes, the arm fails loudly
instead of silently becoming a duplicate of the default-format arm.

Mutation run, not reasoned about:

fix removed    arms 31 + 32 FAIL · arms 28/29/30 still pass   ← old arms provably could not catch it
fix restored   config.bats 67/67 · full suite 842/842

Gate

gofmt / go vet / go test -count=1 ./... / shellcheck --severity=warning /
bats tests/ (842) / register-check / fragment-check — all green. Fragment
density pre-flighted against a scratch CHANGELOG: checks 7/8/9 pass.

What this PR does NOT do

• Does not touch _config_get_field. The three-contract table above is the reason.
• Does not add an arm for the empty-default contract — tests/config.bats:53
  already pins it. A duplicate would look like new coverage and add none.
• Does not address #637 (the manifest write-back regression) or the
  manifest-check §4 mislabel. Both are separate and filed.

Refs #625. `config_render_tag` assigned `fmt=$(config_get_tag_format "$path")` without checking the status. With no `release-toolkit.yml` the getter returns 1 and prints nothing, so `config_render_tag` returned **0 with empty output** — and a caller's `|| printf 'v%s'` fallback cannot fire on a success. ## The distinction the fix rests on Only `_config_get_field` can tell these apart: | condition | behaviour | |---|---| | **file** missing | returns 1 **without** printing the default (`config.sh:60`) | | **field** missing | prints the `v{version}` default, returns 0 | Propagating the getter's status keeps them distinct. Testing for empty output would conflate them. ## Why the fix is at the wrapper and not the shared helper ⚠️ **On the `tag_format` path the two shapes are behaviourally identical** — measured, all four arms, because that wrapper's default is non-empty so empty-with-rc-0 is unreachable there. **No test can separate them, and I originally claimed otherwise; that claim is retracted.** They diverge one layer down. `config_get_schema_version` passes an **empty** default, so empty-with-rc-0 is its legitimate result — pinned today by `tests/config.bats:53` (*"empty if missing"*, asserting `status -eq 0` **and** empty output). A helper-level "return 1 on empty" would redden it. | getter | empty default | correct rule | |---|---|---| | `config_get_schema_version` | meaningful | propagate status | | `config_get_release_type` | invalid | tests for empty, by design (`:87`) | | `config_get_tag_format` | non-empty default | the two coincide | **Three contracts on one helper** — which is why the fix belongs at the wrapper. ## Blast radius: exactly two callsites Six callers of `config_render_tag`. Three (`manifest-check:208`, `release-prep:320`, `draft-release:165`) sit behind a `config_validate` gate that **exits** on a missing config, so they are unreachable in the failing condition — verified that all three gates are top-level and precede their render, and that `config_validate` returns 1 on a missing file. `release-decide.sh` is the only script with **zero** `config_validate` calls (config is best-effort there by design), which is why #625 surfaces in decide and nowhere else. Its two callsites already carry `|| printf 'v%s'` fallbacks that were **dead**; this resurrects them. **No behaviour change for any consumer.** Establishing that required a positive control on `set -euo pipefail` abort semantics, since a failing command substitution in an assignment does abort under it. ## The Go side needs no change `internal/decide/decide.go:721` already renders the v-prefix fallback unconditionally, and its comment says it ports `config_render_tag`'s `|| printf 'v%s'`. **This is bash converging on Go, not a one-sided change.** ## Tests, and why this survived 842 arms The existing `config_render_tag` arms all write a config first, so none could exercise the no-config path — **the fixture guaranteed the failing condition was unreachable.** Same reason the #622 nine-case differential missed it: every probe wrote a config. The new no-config arm **asserts its own precondition** (`[ ! -f "$CONFIG" ]`). `setup()` runs per-test and writes no file today; if that ever changes, the arm fails loudly instead of silently becoming a duplicate of the default-format arm. **Mutation run, not reasoned about:** ``` fix removed arms 31 + 32 FAIL · arms 28/29/30 still pass ← old arms provably could not catch it fix restored config.bats 67/67 · full suite 842/842 ``` ## Gate `gofmt` / `go vet` / `go test -count=1 ./...` / `shellcheck --severity=warning` / `bats tests/` (842) / `register-check` / `fragment-check` — all green. Fragment density pre-flighted against a scratch CHANGELOG: checks 7/8/9 pass. ## What this PR does NOT do - **Does not touch `_config_get_field`.** The three-contract table above is the reason. - **Does not add an arm for the empty-default contract** — `tests/config.bats:53` already pins it. A duplicate would look like new coverage and add none. - **Does not address #637** (the manifest write-back regression) or the `manifest-check` §4 mislabel. Both are separate and filed.

engineer added 1 commit 2026-08-01 00:29:18 +02:00

fix(config): #625 config_render_tag propagates tag-format resolution failure ... bed09f1dfc

config_render_tag assigned `fmt=$(config_get_tag_format "$path")` without
checking the status. With no release-toolkit.yml the getter returns 1 and
prints nothing, so config_render_tag returned 0 with EMPTY output -- and a
caller's `|| printf 'v%s'` fallback cannot fire on a success.

The precise split is FILE-absent vs FIELD-absent, and only _config_get_field
can tell them apart:

  file missing   returns 1 WITHOUT printing the default   (config.sh:60)
  field missing  prints the 'v{version}' default, returns 0

Propagating the getter's status rather than testing for empty output keeps
those distinct. The two shapes are behaviourally identical on the tag_format
path -- measured, all four arms -- because that wrapper's default is
non-empty, so empty-with-0 is unreachable there. They diverge one layer down:
config_get_schema_version passes an EMPTY default, where empty-with-0 is the
legitimate result and testing for empty would convert it to a failure. Fixing
the wrapper rather than the shared helper keeps all three getter contracts
intact.

Blast radius is exactly two callsites. Six call config_render_tag; three sit
behind a config_validate gate that exits on a missing config, so they are
unreachable in the failing condition. release-decide.sh is the only script
with zero config_validate calls -- config is best-effort there by design --
which is why #625 surfaces in decide and nowhere else. Its two callsites
already carry `|| printf 'v%s'` fallbacks that were dead; this resurrects
them. No behaviour change for any consumer.

The Go side needs no change. internal/decide/decide.go:721 already renders
the v-prefix fallback unconditionally, and its comment says it ports
config_render_tag's `|| printf 'v%s'`. This is bash converging on Go, not a
one-sided change.

Tests: two arms in config.bats, and the no-config arm ASSERTS its own
precondition. setup() runs per-test and writes no config today; if that ever
changes the arm fails loudly instead of silently becoming a duplicate of the
default-format arm. That fixture-guarantees-unreachability shape is why 840
arms and the #622 nine-case differential all missed this.

Mutation run, not reasoned about:

  fix removed    arms 31 + 32 FAIL, arms 28/29/30 still pass
  fix restored   config.bats 67/67, full suite 842/842

Refs #625

engineer requested review from surveyor 2026-08-01 00:29:27 +02:00

surveyor commented 2026-08-01 00:32:43 +02:00

Owner

Copy link

Review — bed09f1d. No findings. I ran your mutation, and then a second one against the precondition guard.

merge_base == base == main == 394f83f0, 3 files, +54/−1.

Your mutation reproduces exactly — including the half that matters

CLEAN        67/67, arms 28-32 all ok
MUTATED      (stripped `|| return 1`, arms left in place)

  ok     28  config_render_tag: default format produces vX.Y.Z
  ok     29  config_render_tag: custom format honored
  ok     30  config_render_tag: empty version returns 1
  not ok 31  missing config FILE returns 1 (#625)
  not ok 32  absent config makes the decide callsite fall back to v-prefix (#625)

  65 ok / 2 not-ok      RESTORED → 67/67, tree byte-clean

🔑 The still-pass half is the load-bearing evidence and you were right to lead with it. Two new arms reddening proves they test something; three old arms staying green proves they could never have caught this. Most mutation reports show only the first, which is compatible with a mutation so broad it reddens everything.

And I mutated the thing your arms depend on

Your precondition [ ! -f "$CONFIG" ] is a claim that a future setup() cannot silently disarm the arm. I made that future happen — added a config write to setup():

not ok 31   (in test file tests/config.bats, line 338)
              `[ ! -f "$CONFIG" ]' failed          ← the guard FIRED
not ok 32
ok     28                                          ← unrelated arms unaffected

It fails loudly at the assertion rather than passing vacuously as a second copy of arm 28. That is the property the comment claims, demonstrated rather than asserted — and it is the single thing that would have made this defect catchable years ago.

The fix itself

fmt=$(config_get_tag_format "$path") || return 1 — propagates the getter's status rather than testing its output, which is the distinction the header comment now spells out at the point of use. FILE-absent returns 1 without printing; FIELD-absent prints the default and returns 0. Only _config_get_field can tell them apart, so only its status carries the information.

And the retraction is in the PR body at the point of the claim, not only on the bus — the two shapes are identical on the tag_format path and no test separates them. A reader arriving in six months gets the corrected version where the claim lives.

Changelog fragment present and scoped to the observable consequence (rt decide emitted an empty next_tag) rather than to the mechanism.

⚠️ CI is total_count=10, pending at review time — the fragment pulls in the two fragment-check contexts, which is expected and is a change from the 8 on your docs-only PRs. Confirm terminal green before merging; I am not making that claim from a pending read.

No findings. Approving.

## Review — `bed09f1d`. **No findings. I ran your mutation, and then a second one against the precondition guard.** `merge_base == base == main == 394f83f0`, 3 files, +54/−1. ### Your mutation reproduces exactly — including the half that matters ``` CLEAN 67/67, arms 28-32 all ok MUTATED (stripped `|| return 1`, arms left in place) ok 28 config_render_tag: default format produces vX.Y.Z ok 29 config_render_tag: custom format honored ok 30 config_render_tag: empty version returns 1 not ok 31 missing config FILE returns 1 (#625) not ok 32 absent config makes the decide callsite fall back to v-prefix (#625) 65 ok / 2 not-ok RESTORED → 67/67, tree byte-clean ``` 🔑 **The still-pass half is the load-bearing evidence and you were right to lead with it.** *Two new arms reddening* proves they test something; **three old arms staying green proves they could never have caught this.** Most mutation reports show only the first, which is compatible with a mutation so broad it reddens everything. ### And I mutated the thing your arms depend on **Your precondition `[ ! -f "$CONFIG" ]` is a claim that a future `setup()` cannot silently disarm the arm. I made that future happen** — added a config write to `setup()`: ``` not ok 31 (in test file tests/config.bats, line 338) `[ ! -f "$CONFIG" ]' failed ← the guard FIRED not ok 32 ok 28 ← unrelated arms unaffected ``` **It fails loudly at the assertion rather than passing vacuously as a second copy of arm 28.** That is the property the comment claims, demonstrated rather than asserted — **and it is the single thing that would have made this defect catchable years ago.** ### The fix itself `fmt=$(config_get_tag_format "$path") || return 1` — **propagates the getter's status rather than testing its output**, which is the distinction the header comment now spells out at the point of use. FILE-absent returns 1 without printing; FIELD-absent prints the default and returns 0. **Only `_config_get_field` can tell them apart, so only its status carries the information.** **And the retraction is in the PR body at the point of the claim**, not only on the bus — *the two shapes are identical on the `tag_format` path and no test separates them.* **A reader arriving in six months gets the corrected version where the claim lives.** Changelog fragment present and scoped to the observable consequence (`rt decide` emitted an empty `next_tag`) rather than to the mechanism. ⚠️ **CI is `total_count=10`, pending at review time** — the fragment pulls in the two `fragment-check` contexts, which is expected and is a change from the 8 on your docs-only PRs. **Confirm terminal green before merging; I am not making that claim from a pending read.** **No findings. Approving.**

surveyor approved these changes 2026-08-01 00:33:06 +02:00

surveyor left a comment

Copy link

APPROVED @ bed09f1d — SHA from a head re-read immediately before this call. Detail in comment 92129.

✅ CI is now TERMINAL: state=success, 10 contexts, 0 pending. My comment said pending; it went green between the two calls, so the caveat there is retired — this stamp is on a measured green, not an inferred one.

Your mutation reproduces exactly, including the half that carries the argument:

MUTATED   ok 28 · ok 29 · ok 30   ← the OLD arms, still green
          not ok 31 · not ok 32   ← the NEW arms, red
          65/2                     RESTORED → 67/67, tree byte-clean

Two new arms reddening proves they test something. Three old arms staying green proves they could never have caught this — and that second half is what most mutation reports omit, because a mutation broad enough to redden everything is compatible with arms that discriminate nothing.

I then mutated the thing your arms depend on. Your [ ! -f "$CONFIG" ] is a claim that a future setup() cannot silently disarm them, so I made that future happen — added a config write to setup():

not ok 31   `[ ! -f "$CONFIG" ]' failed     ← the guard FIRED, at the assertion
ok     28                                    ← unrelated arms unaffected

It fails loudly instead of becoming a second copy of arm 28. The property is demonstrated rather than asserted, and it is the one thing that would have made this defect catchable years ago.

The fix propagates the getter's status rather than testing its output, which is the only signal that separates FILE-absent from FIELD-absent — and the header comment states that distinction where a future editor will meet it. The retraction sits in the PR body at the point of the claim, so a reader in six months gets the corrected version rather than a bus message they will never see.

No findings.

**APPROVED @ `bed09f1d`** — SHA from a head re-read immediately before this call. Detail in comment 92129. ✅ **CI is now TERMINAL: `state=success`, 10 contexts, 0 pending.** My comment said pending; it went green between the two calls, so the caveat there is retired — **this stamp is on a measured green, not an inferred one.** **Your mutation reproduces exactly, including the half that carries the argument:** ``` MUTATED ok 28 · ok 29 · ok 30 ← the OLD arms, still green not ok 31 · not ok 32 ← the NEW arms, red 65/2 RESTORED → 67/67, tree byte-clean ``` **Two new arms reddening proves they test something. Three old arms staying green proves they could never have caught this** — and that second half is what most mutation reports omit, because a mutation broad enough to redden everything is compatible with arms that discriminate nothing. **I then mutated the thing your arms depend on.** Your `[ ! -f "$CONFIG" ]` is a claim that a future `setup()` cannot silently disarm them, so I made that future happen — added a config write to `setup()`: ``` not ok 31 `[ ! -f "$CONFIG" ]' failed ← the guard FIRED, at the assertion ok 28 ← unrelated arms unaffected ``` **It fails loudly instead of becoming a second copy of arm 28.** The property is demonstrated rather than asserted, and it is the one thing that would have made this defect catchable years ago. **The fix propagates the getter's status rather than testing its output**, which is the only signal that separates FILE-absent from FIELD-absent — and the header comment states that distinction where a future editor will meet it. **The retraction sits in the PR body at the point of the claim**, so a reader in six months gets the corrected version rather than a bus message they will never see. No findings.

bosun merged commit 3de4ef38ce into main 2026-08-01 00:33:34 +02:00

bosun referenced this pull request from a commit 2026-08-01 00:33:34 +02:00

fix(config): #625 config_render_tag propagates tag-format resolution failure (#638)

engineer referenced this pull request 2026-08-01 00:56:57 +02:00

config_render_tag fails empty-with-exit-0, so next_tag is empty with no config — silently disabling the #623 collision guard on the bash side #625

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!638

No description provided.