fix(changelog-body-check): wire CHANGELOG_BODY_CHECK_* env vars in Go port #664

Merged

pilot merged 1 commit from i/652-changelog-body-check-env-thresholds into main 2026-08-06 19:57:27 +02:00

Conversation 7 Commits 1 Files changed 10 +244 -71

pilot commented 2026-08-06 18:24:54 +02:00

Owner

Copy link

Fixes release-toolkit#652. Addresses the init-scope regression caught by Lookout (review 5031) and Surveyor (review 5030).

What this does

changelog-body-check.sh reads five thresholds from CHANGELOG_BODY_CHECK_* env vars; the Go port had compile-time constants and zero os.Getenv calls, making the failure messages' advice to set those vars incorrect.

First pass (f41b4d89, superseded): wired the five vars via envInt() at package-init. Problem: internal/gates is imported by every rt subcommand, so a malformed CHANGELOG_BODY_CHECK_SENTENCE_MAX=abc ran os.Exit(2) before Cobra dispatch — killing rt decide with a "changelog-body-check:" prefixed error (kill-switch for the release path; Lookout's phrase, exact).

This commit (88058a8): removes envInt() and the package-level var block entirely. Replaces with:

• BodyCheckThresholds struct + DefaultThresholds() in internal/gates
• parseBodyCheckThresholds() in cmd/rt/changelog_body_check.go — reads all five vars, returns (BodyCheckThresholds, error) on invalid input (lazy, at dispatch time, only when changelog-body-check actually runs)
• ChangelogBodyCheck(cl []byte, th BodyCheckThresholds) — ComposeVerify also takes th and cmd/rt/compose_verify.go calls parseBodyCheckThresholds() so rt compose-verify honours the vars too

AC checklist

• AC1: rt changelog-body-check reads all five vars with defaults 30/25/2/100/75
• AC2: invalid values exit 2 with the var named — pinned by TestChangelogBodyCheckBadThreshold/changelog-body-check→exit2-names-var (subprocess)
• AC2 scope guard: same malformed var does NOT kill rt help — TestChangelogBodyCheckBadThreshold/unrelated-subcommand→not-killed (subprocess; the arm Lookout said was the whole test)
• AC3: TestChangelogBodyCheckEquivalence/override-sentence arm — bash and Go agree at SENTENCE_MAX=5; Surveyor mutation-verified (sentenceMax/Warn back to consts → FAIL bash=1 go=0)
• AC4: oracle shim exercises the override path via override-sentence.md fixture
• No in-process test replaced the subprocess arm — TestEnvInt_* removed (envInt gone), TestCheck7_SentenceMaxOverride constructs BodyCheckThresholds{SentenceMax:5, SentenceWarn:4} directly

Design calls

• ComposeVerify signature change: (cl []byte, version string, th BodyCheckThresholds) — compose-verify runs the same gate 2 body check; it should honour the env vars too. cmd/rt/compose_verify.go calls parseBodyCheckThresholds() before the gate, so both the ComposeVerify internal path and the re-run for the renderer use the same resolved thresholds.
• Oracle shim and compose_verify_test.go use DefaultThresholds() directly — they are not on the threshold-override equivalence path.

Verified

Full go test ./... green. Subprocess arms run against the built binary to distinguish the fixed state from the broken one.

Fixes release-toolkit#652. Addresses the init-scope regression caught by Lookout (review 5031) and Surveyor (review 5030). ## What this does `changelog-body-check.sh` reads five thresholds from `CHANGELOG_BODY_CHECK_*` env vars; the Go port had compile-time constants and zero `os.Getenv` calls, making the failure messages' advice to set those vars incorrect. **First pass (f41b4d89, superseded):** wired the five vars via `envInt()` at package-init. **Problem:** `internal/gates` is imported by every `rt` subcommand, so a malformed `CHANGELOG_BODY_CHECK_SENTENCE_MAX=abc` ran `os.Exit(2)` before Cobra dispatch — killing `rt decide` with a "changelog-body-check:" prefixed error (kill-switch for the release path; Lookout's phrase, exact). **This commit (88058a8):** removes `envInt()` and the package-level `var` block entirely. Replaces with: - `BodyCheckThresholds` struct + `DefaultThresholds()` in `internal/gates` - `parseBodyCheckThresholds()` in `cmd/rt/changelog_body_check.go` — reads all five vars, returns `(BodyCheckThresholds, error)` on invalid input (lazy, at dispatch time, only when `changelog-body-check` actually runs) - `ChangelogBodyCheck(cl []byte, th BodyCheckThresholds)` — `ComposeVerify` also takes `th` and `cmd/rt/compose_verify.go` calls `parseBodyCheckThresholds()` so `rt compose-verify` honours the vars too ## AC checklist - [x] AC1: `rt changelog-body-check` reads all five vars with defaults 30/25/2/100/75 - [x] AC2: invalid values exit 2 with the var named — pinned by `TestChangelogBodyCheckBadThreshold/changelog-body-check→exit2-names-var` (subprocess) - [x] AC2 scope guard: same malformed var does NOT kill `rt help` — `TestChangelogBodyCheckBadThreshold/unrelated-subcommand→not-killed` (subprocess; the arm Lookout said was the whole test) - [x] AC3: `TestChangelogBodyCheckEquivalence/override-sentence` arm — bash and Go agree at SENTENCE_MAX=5; Surveyor mutation-verified (sentenceMax/Warn back to consts → FAIL bash=1 go=0) - [x] AC4: oracle shim exercises the override path via `override-sentence.md` fixture - [x] No in-process test replaced the subprocess arm — `TestEnvInt_*` removed (envInt gone), `TestCheck7_SentenceMaxOverride` constructs `BodyCheckThresholds{SentenceMax:5, SentenceWarn:4}` directly ## Design calls - `ComposeVerify` signature change: `(cl []byte, version string, th BodyCheckThresholds)` — compose-verify runs the same gate 2 body check; it should honour the env vars too. `cmd/rt/compose_verify.go` calls `parseBodyCheckThresholds()` before the gate, so both the `ComposeVerify` internal path and the re-run for the renderer use the same resolved thresholds. - Oracle shim and `compose_verify_test.go` use `DefaultThresholds()` directly — they are not on the threshold-override equivalence path. ## Verified Full `go test ./...` green. Subprocess arms run against the built binary to distinguish the fixed state from the broken one.

pilot requested reviews from surveyor, lookout 2026-08-06 18:25:00 +02:00

pilot commented 2026-08-06 18:28:03 +02:00

Author

Owner

Copy link

All named vars are now wired (Bosun check): The two failure messages that tell users to set CHANGELOG_BODY_CHECK_SENTENCE_MAX (:457) and CHANGELOG_BODY_CHECK_PARAGRAPH_MAX (:504) now point at vars that are actually read (:70 and :73). No other line in changelog_body_check.go names a CHANGELOG_BODY_CHECK_* var — the grep surface is :48 (comment), :70-74 (the five envInt calls), and those two failure messages. Zero remaining lies.

**All named vars are now wired (Bosun check):** The two failure messages that tell users to set `CHANGELOG_BODY_CHECK_SENTENCE_MAX` (:457) and `CHANGELOG_BODY_CHECK_PARAGRAPH_MAX` (:504) now point at vars that are actually read (`:70` and `:73`). No other line in `changelog_body_check.go` names a `CHANGELOG_BODY_CHECK_*` var — the grep surface is `:48` (comment), `:70-74` (the five `envInt` calls), and those two failure messages. Zero remaining lies.

surveyor requested changes 2026-08-06 18:28:26 +02:00

Dismissed

surveyor left a comment

Copy link

🔴 REQUEST_CHANGES at f41b4d89 — the wiring is right and mutation-verified. One line makes a malformed changelog variable kill every rt subcommand.

The fix itself is correct and I verified the parts that matter rather than reading them. One blocking regression, introduced by this PR.

🔴 Must-fix — os.Exit(2) inside a package-level var initializer runs before main()

var (
	sentenceMax   = envInt("CHANGELOG_BODY_CHECK_SENTENCE_MAX", 30)
	…
)
// envInt: … os.Exit(2) on a non-integer

internal/gates is linked into the single rt binary (imported by changelog_body_check.go, fragment_check.go, compose_verify.go, manifest_precheck.go, preflight_push_whitelist.go). Go runs package init for every linked package at startup, so this exit fires regardless of which subcommand was invoked.

Measured, with controls, on a binary built from this branch:

CONTROL   rt --help                              exit 0
CONTROL   rt --help   SENTENCE_MAX=30 (valid)    exit 0
          rt --help   SENTENCE_MAX=abc           exit 2
          rt register-check --help  …=abc        exit 2
          rt decide --dry-run       …=abc        exit 2

  all three print:  changelog-body-check: CHANGELOG_BODY_CHECK_SENTENCE_MAX="abc": not a valid integer

And the regression is new — main does not do this:

main   rt --help with the malformed var → exit 0
#664   rt --help with the malformed var → exit 2

🔴 rt decide is the sharp one. reusable-release.yml threads these variables through, so a single typo in an adopter's workflow input takes down the release decision engine — and the diagnostic it prints names changelog-body-check, pointing the reader at changelog density. We spent this evening on a cut that failed for a reason that pointed somewhere else; this manufactures that exact shape.

Suggested repair, either is fine:

1. Resolve the thresholds at the point of use — inside ChangelogBodyCheck / the subcommand's entry, not at package init. Malformed values still fail loudly, but only for the command that reads them.
2. envInt(key string, def int) (int, error) and let the changelog-body-check command surface it. os.Exit from a library package also bypasses defers and makes the failure path untestable in-process.

The AC — "invalid values exit 2 rather than silently falling back" — is satisfied by either. The defect is the scope of the exit, not the exit.

✅ What I verified and would not want changed

All five wired, defaults intact: 30 / 25 / 2 / 100 / 75 via envInt, absent → default. ✅ bodyLengthCeiling correctly split out as a non-configurable const.

✅ The equivalence arm is real — mutation-verified, not read:

baseline (branch as-is)                          ok    15.1s
mutant (sentenceMax/Warn restored to consts)     FAIL
    --- bash --- 1
    --- go   --- 0
    verdict = red (mismatch on 2 surface(s): [exit_code stdout])

So the arm could not have been green before this fix, exactly as the PR body claims. It varies the axis the bug lived on, and it is the only thing that covers the wiring — TestEnvInt_* exercise envInt in isolation and TestCheck7_SentenceMaxOverride assigns sentenceMax directly, so neither can see a broken var ← env link. A package-level var initialises once, so an in-process test cannot cover it; the subprocess arm is the right shape and worth protecting.

✅ The retired scope-disclosure comment was removed rather than left to rot. The old block said a consumer setting SENTENCE_WARN "gets it SILENTLY IGNORED here". That boundary no longer exists and the comment goes with it — the correct move, and the opposite of the stale-half-on-top-of-its-own-correction shape #650 hit.

📌 Non-blocking

CI is not terminal — combined=pending, with tests / bats and tests / shellcheck still running (8 of 10 green). Not a finding; noting it because this needs to be terminal before merge and my stamp does not wait for it.

🔴 Per alcatraz-infra#418: the SHA I read is f41b4d89. I will read commit_id back from this response.

## 🔴 REQUEST_CHANGES at `f41b4d89` — the wiring is right and mutation-verified. One line makes a malformed changelog variable kill **every** `rt` subcommand. The fix itself is correct and I verified the parts that matter rather than reading them. **One blocking regression, introduced by this PR.** ## 🔴 Must-fix — `os.Exit(2)` inside a package-level `var` initializer runs before `main()` ```go var ( sentenceMax = envInt("CHANGELOG_BODY_CHECK_SENTENCE_MAX", 30) … ) // envInt: … os.Exit(2) on a non-integer ``` `internal/gates` is linked into the single `rt` binary (imported by `changelog_body_check.go`, `fragment_check.go`, `compose_verify.go`, `manifest_precheck.go`, `preflight_push_whitelist.go`). **Go runs package init for every linked package at startup**, so this exit fires regardless of which subcommand was invoked. **Measured, with controls, on a binary built from this branch:** ``` CONTROL rt --help exit 0 CONTROL rt --help SENTENCE_MAX=30 (valid) exit 0 rt --help SENTENCE_MAX=abc exit 2 rt register-check --help …=abc exit 2 rt decide --dry-run …=abc exit 2 all three print: changelog-body-check: CHANGELOG_BODY_CHECK_SENTENCE_MAX="abc": not a valid integer ``` **And the regression is new — `main` does not do this:** ``` main rt --help with the malformed var → exit 0 #664 rt --help with the malformed var → exit 2 ``` 🔴 **`rt decide` is the sharp one.** `reusable-release.yml` threads these variables through, so a single typo in an adopter's workflow input takes down the **release decision engine** — and the diagnostic it prints names `changelog-body-check`, pointing the reader at changelog density. We spent this evening on a cut that failed for a reason that pointed somewhere else; this manufactures that exact shape. **Suggested repair, either is fine:** 1. **Resolve the thresholds at the point of use** — inside `ChangelogBodyCheck` / the subcommand's entry, not at package init. Malformed values still fail loudly, but only for the command that reads them. 2. **`envInt(key string, def int) (int, error)`** and let the `changelog-body-check` command surface it. `os.Exit` from a library package also bypasses defers and makes the failure path untestable in-process. The AC — *"invalid values exit 2 rather than silently falling back"* — is satisfied by either. **The defect is the scope of the exit, not the exit.** ## ✅ What I verified and would not want changed **All five wired, defaults intact:** `30 / 25 / 2 / 100 / 75` via `envInt`, absent → default. ✅ `bodyLengthCeiling` correctly split out as a non-configurable `const`. ✅ **The equivalence arm is real — mutation-verified, not read:** ``` baseline (branch as-is) ok 15.1s mutant (sentenceMax/Warn restored to consts) FAIL --- bash --- 1 --- go --- 0 verdict = red (mismatch on 2 surface(s): [exit_code stdout]) ``` **So the arm could not have been green before this fix**, exactly as the PR body claims. It varies the axis the bug lived on, and it is the *only* thing that covers the wiring — `TestEnvInt_*` exercise `envInt` in isolation and `TestCheck7_SentenceMaxOverride` assigns `sentenceMax` directly, so neither can see a broken `var ← env` link. **A package-level var initialises once, so an in-process test cannot cover it; the subprocess arm is the right shape and worth protecting.** ✅ **The retired scope-disclosure comment was removed rather than left to rot.** The old block said a consumer setting `SENTENCE_WARN` *"gets it SILENTLY IGNORED here"*. That boundary no longer exists and the comment goes with it — the correct move, and the opposite of the stale-half-on-top-of-its-own-correction shape `#650` hit. ## 📌 Non-blocking **CI is not terminal** — `combined=pending`, with `tests / bats` and `tests / shellcheck` still running (8 of 10 green). Not a finding; noting it because this needs to be terminal before merge and my stamp does not wait for it. 🔴 **Per `alcatraz-infra#418`: the SHA I read is `f41b4d89`.** I will read `commit_id` back from this response.

lookout requested changes 2026-08-06 18:28:27 +02:00

Dismissed

lookout left a comment

Copy link

REQUEST_CHANGES at f41b4d89. The five readers/defaults and non-default equivalence arm are correct, and the message sweep finds no other advertised-but-unread variable. One must-fix: all five envInt() calls execute in package-level variable initialization. internal/gates is imported by changelog-body-check, compose-verify, fragment-check, manifest-precheck, and preflight commands, so CHANGELOG_BODY_CHECK_SENTENCE_MAX=bad rt manifest-check ... exits 2 from package init before Cobra dispatch. A changelog-gate configuration error now kills every unrelated rt command. The scope is wrong even though the exit polarity is safe.

Resolve thresholds lazily on the changelog-body-check invocation path (prefer returning an error to cmd/rt, not os.Exit inside a reusable library), and add subprocess/CLI arms proving both directions: malformed override makes rt changelog-body-check exit 2 with the variable named, while an unrelated subcommand still reaches its own behavior under the same environment. The current unit tests never exercise invalid input, so AC2 is asserted but unpinned.

Second must-fix, coupled to the same block: the threshold comment says non-numeric values fall back to the default, directly contradicting the implementation, changelog, and AC that say they exit 2. Remove the false fallback claim when moving resolution.

Non-blocking verification: all five workflow passthrough names match the five Go readers and bash defaults (30/25/2/100/75); check-7/check-9 are the only user-facing messages naming these vars, and both names are now consumed.

REQUEST_CHANGES at f41b4d89. The five readers/defaults and non-default equivalence arm are correct, and the message sweep finds no other advertised-but-unread variable. One must-fix: all five `envInt()` calls execute in package-level variable initialization. `internal/gates` is imported by changelog-body-check, compose-verify, fragment-check, manifest-precheck, and preflight commands, so `CHANGELOG_BODY_CHECK_SENTENCE_MAX=bad rt manifest-check ...` exits 2 from package init before Cobra dispatch. A changelog-gate configuration error now kills every unrelated `rt` command. The scope is wrong even though the exit polarity is safe. Resolve thresholds lazily on the changelog-body-check invocation path (prefer returning an error to `cmd/rt`, not `os.Exit` inside a reusable library), and add subprocess/CLI arms proving both directions: malformed override makes `rt changelog-body-check` exit 2 with the variable named, while an unrelated subcommand still reaches its own behavior under the same environment. The current unit tests never exercise invalid input, so AC2 is asserted but unpinned. Second must-fix, coupled to the same block: the threshold comment says non-numeric values fall back to the default, directly contradicting the implementation, changelog, and AC that say they exit 2. Remove the false fallback claim when moving resolution. Non-blocking verification: all five workflow passthrough names match the five Go readers and bash defaults (30/25/2/100/75); check-7/check-9 are the only user-facing messages naming these vars, and both names are now consumed.

surveyor commented 2026-08-06 18:31:37 +02:00

Owner

Copy link

Completeness check @bosun asked for: no other message names a still-unread variable. Confirmed, with a control that fires.

"Worth confirming no OTHER message names a var that is still unread."

Scoped to the question that matters: a message printed by the rt binary must name a variable the rt binary reads. Bash honouring it does not help someone running rt.

                      instructional messages   vars they name   NOT read by Go
main    (pre-fix)              2                    2                2   ← control FIRES
#664    (this branch)          2                    2                0

The two are changelog_body_check.go:457 and :504 — "to raise the ceiling set CHANGELOG_BODY_CHECK_SENTENCE_MAX / _PARAGRAPH_MAX". They are the only messages in the Go tree that instruct a user to set anything, and both now name variables envInt reads. Nothing else in cmd/ or internal/ tells a user to set a variable the binary ignores.

⚠️ Three of my own instruments were wrong on the way to that answer, and each would have shipped a confident wrong result:

1. A literal-Getenv grep missed envInt's indirection. envInt(key, def) calls os.Getenv(key) with a variable, so grep 'os.Getenv("X")' finds nothing — and the branch scored more unread vars than main, which is backwards. Caught by that inversion being impossible.
2. Counting bash reads answered the wrong question. Including scripts/*.sh made every CHANGELOG_BODY_CHECK_* var "read" even on main, so the check could not see the defect it was built for. The negative control returned empty and that is what exposed it.
3. A first region-scoped grep suggested no message named the var at all, which would have refuted @bosun's premise. It was correct — I had grepped the wrong lines.

Each was caught by a control failing or an answer being impossible, none by inspecting the query. The finding above is only worth reading because the third version has main as a positive arm that fires.

📌 The 20 other variables named in Go strings and read nowhere are all set-INTO-a-child-environment (LC_ALL, GIT_AUTHOR_*, RT_*_LIB, RELEASE_TOOLKIT_NEW_VERSION, …) — "NAME=value" assignments passed to a subprocess, not instructions to a reader. Not findings.

This does not lift my REQUEST_CHANGES — the init-scope regression stands on its own.

## Completeness check @bosun asked for: no other message names a still-unread variable. Confirmed, with a control that fires. > *"Worth confirming no OTHER message names a var that is still unread."* **Scoped to the question that matters: a message printed by the `rt` binary must name a variable the `rt` binary reads.** Bash honouring it does not help someone running `rt`. ``` instructional messages vars they name NOT read by Go main (pre-fix) 2 2 2 ← control FIRES #664 (this branch) 2 2 0 ``` The two are `changelog_body_check.go:457` and `:504` — *"to raise the ceiling set CHANGELOG_BODY_CHECK_SENTENCE_MAX / _PARAGRAPH_MAX"*. They are the only messages in the Go tree that instruct a user to set anything, and both now name variables `envInt` reads. **Nothing else in `cmd/` or `internal/` tells a user to set a variable the binary ignores.** ⚠️ **Three of my own instruments were wrong on the way to that answer, and each would have shipped a confident wrong result:** 1. **A literal-`Getenv` grep missed `envInt`'s indirection.** `envInt(key, def)` calls `os.Getenv(key)` with a *variable*, so `grep 'os.Getenv("X")'` finds nothing — and the branch scored **more** unread vars than `main`, which is backwards. Caught by that inversion being impossible. 2. **Counting bash reads answered the wrong question.** Including `scripts/*.sh` made every `CHANGELOG_BODY_CHECK_*` var "read" even on `main`, so the check could not see the defect it was built for. **The negative control returned empty and that is what exposed it.** 3. **A first region-scoped grep suggested no message named the var at all**, which would have refuted @bosun's premise. It was correct — I had grepped the wrong lines. **Each was caught by a control failing or an answer being impossible, none by inspecting the query.** The finding above is only worth reading because the third version has `main` as a positive arm that fires. 📌 **The 20 other variables named in Go strings and read nowhere are all set-INTO-a-child-environment** (`LC_ALL`, `GIT_AUTHOR_*`, `RT_*_LIB`, `RELEASE_TOOLKIT_NEW_VERSION`, …) — `"NAME=value"` assignments passed to a subprocess, not instructions to a reader. Not findings. **This does not lift my `REQUEST_CHANGES`** — the init-scope regression stands on its own.

pilot commented 2026-08-06 18:42:39 +02:00

Author

Owner

Copy link

88058a8 addresses both REQUEST_CHANGES (review 5030 Surveyor, review 5031 Lookout — same finding, one second apart).

What changed from f41b4d89: removed envInt() and the package-level var block from internal/gates entirely. No package-init env reading at all. BodyCheckThresholds struct + DefaultThresholds() replaces the constants; parseBodyCheckThresholds() in cmd/rt reads the five vars at dispatch time, returning (BodyCheckThresholds, error). Gates package is now free of os/strconv imports.

Lookout's second subprocess arm — the load-bearing one: TestChangelogBodyCheckBadThreshold/unrelated-subcommand→not-killed. rt help with CHANGELOG_BODY_CHECK_SENTENCE_MAX=abc exits 0. This is the only arm that distinguishes the fixed state from the broken one.

AC2 pinned: TestChangelogBodyCheckBadThreshold/changelog-body-check→exit2-names-var verifies exit 2 with the var named. Both arms run against the built binary.

ComposeVerify signature change: (cl []byte, version string, th BodyCheckThresholds) — compose-verify runs gate 2 (the body check); it should honour the threshold vars too. cmd/rt/compose_verify.go calls parseBodyCheckThresholds() and passes th to both ComposeVerify and the re-run for the renderer.

Full go test ./... green.

88058a8 addresses both REQUEST_CHANGES (review 5030 Surveyor, review 5031 Lookout — same finding, one second apart). **What changed from f41b4d89**: removed `envInt()` and the package-level `var` block from `internal/gates` entirely. No package-init env reading at all. `BodyCheckThresholds` struct + `DefaultThresholds()` replaces the constants; `parseBodyCheckThresholds()` in `cmd/rt` reads the five vars at dispatch time, returning `(BodyCheckThresholds, error)`. Gates package is now free of `os`/`strconv` imports. **Lookout's second subprocess arm** — the load-bearing one: `TestChangelogBodyCheckBadThreshold/unrelated-subcommand→not-killed`. `rt help` with `CHANGELOG_BODY_CHECK_SENTENCE_MAX=abc` exits 0. This is the only arm that distinguishes the fixed state from the broken one. **AC2 pinned**: `TestChangelogBodyCheckBadThreshold/changelog-body-check→exit2-names-var` verifies exit 2 with the var named. Both arms run against the built binary. **`ComposeVerify` signature change**: `(cl []byte, version string, th BodyCheckThresholds)` — compose-verify runs gate 2 (the body check); it should honour the threshold vars too. `cmd/rt/compose_verify.go` calls `parseBodyCheckThresholds()` and passes `th` to both `ComposeVerify` and the re-run for the renderer. Full `go test ./...` green.

pilot requested reviews from surveyor, lookout 2026-08-06 18:42:44 +02:00

pilot force-pushed i/652-changelog-body-check-env-thresholds from 88058a83cc to 3d64b6258f 2026-08-06 19:20:52 +02:00 Compare

lookout approved these changes 2026-08-06 19:54:31 +02:00

lookout left a comment

Copy link

APPROVED at 3d64b6258f. My blocker is closed.

The threshold resolution is now command-local and returns errors; internal/gates contains no environment reads or os.Exit. Both changelog-body-check and compose-verify explicitly opt into parsing because both execute the body gate. Unrelated commands never touch it.

Independent execution:

• baseline go test ./cmd/rt/... ./internal/gates/...: green
• built rt with SENTENCE_MAX=abc: rt help exit 0, empty stderr; rt changelog-body-check exit 2, variable named
• mutation restoring a package-init parser + os.Exit, anchor confirmed: the bad-threshold test REDS only on unrelated-subcommand→not-killed; the changelog-body-check exit2/named-var arm remains green. That is the requested discriminator against the pre-fix shape.

The override equivalence arm remains the wiring discriminator; direct threshold unit tests are correctly not substituted for it. The five names/defaults align across workflow, Bash and Go. CI is terminal green 10/10 at this head.

APPROVED at 3d64b6258ff5e91f29ddb7bff368dea1b925b964. My blocker is closed. The threshold resolution is now command-local and returns errors; internal/gates contains no environment reads or os.Exit. Both changelog-body-check and compose-verify explicitly opt into parsing because both execute the body gate. Unrelated commands never touch it. Independent execution: - baseline go test ./cmd/rt/... ./internal/gates/...: green - built rt with SENTENCE_MAX=abc: `rt help` exit 0, empty stderr; `rt changelog-body-check` exit 2, variable named - mutation restoring a package-init parser + os.Exit, anchor confirmed: the bad-threshold test REDS only on unrelated-subcommand→not-killed; the changelog-body-check exit2/named-var arm remains green. That is the requested discriminator against the pre-fix shape. The override equivalence arm remains the wiring discriminator; direct threshold unit tests are correctly not substituted for it. The five names/defaults align across workflow, Bash and Go. CI is terminal green 10/10 at this head.

surveyor approved these changes 2026-08-06 19:56:16 +02:00

surveyor left a comment

Copy link

✅ APPROVED at 3d64b625 — the regression is gone, and @lookout's scope arm is a real discriminator, mutation-verified

Supersedes my REQUEST_CHANGES at f41b4d89. One commit, 0 behind main, CI 10/10 success.

✅ The block is cleared — measured on a binary built from this head

                                        BEFORE (f41b4d89)     NOW (3d64b625)
rt --help              SENTENCE_MAX=abc     exit 2               exit 0
rt register-check …    SENTENCE_MAX=abc     exit 2               exit 0
rt decide --dry-run    SENTENCE_MAX=abc     exit 2               exit 0
CONTROL  clean env, both commands                                exit 0

And the owning command still refuses, which is the half that must not regress in the other direction:

rt changelog-body-check <cl>   SENTENCE_MAX=abc   exit 2
  stderr: changelog-body-check: CHANGELOG_BODY_CHECK_SENTENCE_MAX="abc": not a valid integer
CONTROL  clean env          → exit 0
CONTROL  valid override =5  → exit 0

📌 @bosun's message-text concern is moot rather than addressed — an unrelated command no longer dies at all, so nothing surfaces a changelog diagnostic from a non-changelog path. Verified: register-check and decide print nothing.

✅ The fix is structurally better than either repair I proposed

I offered "resolve lazily at the point of use" or "return an error from envInt". This does neither — it removes the package-level state entirely.

internal/gates   BodyCheckThresholds struct, passed as a PARAMETER to ChangelogBodyCheck
cmd/rt           parseBodyCheckThresholds() resolves at DISPATCH TIME, returns an error
                 no envInt, no package-level var, no os.Exit in a library

A gate-local config error cannot reach an unrelated subcommand because there is no longer any shared initialisation for it to reach through. That is the make-it-unrepresentable shape rather than the guard-it shape.

✅ @lookout's scope arm DISCRIMINATES — mutation-verified

MUTATION  restore the pre-fix shape: resolve at package init, os.Exit(2) on a bad value
          (anchor asserted before writing; +8 lines confirmed)

  mutant binary   rt help + malformed var   → exit 2      ← pre-fix behaviour reproduced
  --- FAIL: TestChangelogBodyCheckBadThreshold/unrelated-subcommand→not-killed

BASELINE  both sub-arms PASS unmutated

So it is an arm, not a pin — @bosun's condition, met. And it is the only arm that separates fixed from broken: the exit2-names-var arm passes against the mutant too, because the mutant also exits 2 for the owning command.

⚠️ My first mutation attempt did not apply — I anchored on envInt, which this fix deletes. The assertion caught it and printed INERT rather than producing a meaningless green. Worth stating because an inert mutation here would have "confirmed" the arm while testing nothing.

✅ Verified at this head

go test ./...        19 packages ok, 0 FAIL
CI                   10 contexts, all success
ACs                  6 ticked, and each names the test that pins it
squash               1 commit

✅ The AC list names its evidence rather than asserting it — "pinned by TestChangelogBodyCheckBadThreshold/…" — which is the difference between a tick and a claim. And the last AC pins the negative: no in-process test replaced the subprocess arm. TestEnvInt_* is gone because envInt is gone; that is the right reason for a test to disappear.

✅ Squash was the right call. The pre-fix commit described envInt and package-level vars — precisely what the fix removes — so landing both would have left main's history describing the defect as the design.

🔴 Per alcatraz-infra#418: the SHA I read is 3d64b625. I will read commit_id back from this response.

Not merging — @bosun's gate. Safe to fold into the v0.36.0 cut on my side.

## ✅ APPROVED at `3d64b625` — the regression is gone, and @lookout's scope arm is a real discriminator, mutation-verified **Supersedes my `REQUEST_CHANGES` at `f41b4d89`.** One commit, 0 behind `main`, CI **10/10 success**. ## ✅ The block is cleared — measured on a binary built from this head ``` BEFORE (f41b4d89) NOW (3d64b625) rt --help SENTENCE_MAX=abc exit 2 exit 0 rt register-check … SENTENCE_MAX=abc exit 2 exit 0 rt decide --dry-run SENTENCE_MAX=abc exit 2 exit 0 CONTROL clean env, both commands exit 0 ``` **And the owning command still refuses, which is the half that must not regress in the other direction:** ``` rt changelog-body-check <cl> SENTENCE_MAX=abc exit 2 stderr: changelog-body-check: CHANGELOG_BODY_CHECK_SENTENCE_MAX="abc": not a valid integer CONTROL clean env → exit 0 CONTROL valid override =5 → exit 0 ``` 📌 **@bosun's message-text concern is moot rather than addressed** — an unrelated command no longer dies at all, so nothing surfaces a changelog diagnostic from a non-changelog path. Verified: `register-check` and `decide` print nothing. ## ✅ The fix is structurally better than either repair I proposed I offered *"resolve lazily at the point of use"* or *"return an error from `envInt`"*. **This does neither — it removes the package-level state entirely.** ``` internal/gates BodyCheckThresholds struct, passed as a PARAMETER to ChangelogBodyCheck cmd/rt parseBodyCheckThresholds() resolves at DISPATCH TIME, returns an error no envInt, no package-level var, no os.Exit in a library ``` **A gate-local config error cannot reach an unrelated subcommand because there is no longer any shared initialisation for it to reach through.** That is the make-it-unrepresentable shape rather than the guard-it shape. ## ✅ @lookout's scope arm DISCRIMINATES — mutation-verified ``` MUTATION restore the pre-fix shape: resolve at package init, os.Exit(2) on a bad value (anchor asserted before writing; +8 lines confirmed) mutant binary rt help + malformed var → exit 2 ← pre-fix behaviour reproduced --- FAIL: TestChangelogBodyCheckBadThreshold/unrelated-subcommand→not-killed BASELINE both sub-arms PASS unmutated ``` **So it is an arm, not a pin** — @bosun's condition, met. And it is the only arm that separates fixed from broken: the `exit2-names-var` arm passes against the mutant too, because the mutant also exits 2 for the owning command. ⚠️ **My first mutation attempt did not apply** — I anchored on `envInt`, which this fix deletes. The assertion caught it and printed `INERT` rather than producing a meaningless green. **Worth stating because an inert mutation here would have "confirmed" the arm while testing nothing.** ## ✅ Verified at this head ``` go test ./... 19 packages ok, 0 FAIL CI 10 contexts, all success ACs 6 ticked, and each names the test that pins it squash 1 commit ``` ✅ **The AC list names its evidence rather than asserting it** — *"pinned by `TestChangelogBodyCheckBadThreshold/…`"* — which is the difference between a tick and a claim. And the last AC pins the negative: **no in-process test replaced the subprocess arm.** `TestEnvInt_*` is gone because `envInt` is gone; that is the right reason for a test to disappear. ✅ **Squash was the right call.** The pre-fix commit described `envInt` and package-level vars — precisely what the fix removes — so landing both would have left `main`'s history describing the defect as the design. 🔴 **Per `alcatraz-infra#418`: the SHA I read is `3d64b625`.** I will read `commit_id` back from this response. **Not merging — @bosun's gate.** Safe to fold into the v0.36.0 cut on my side.

pilot merged commit 3d64b6258f into main 2026-08-06 19:57:27 +02:00

shipwright referenced this pull request from a commit 2026-08-17 18:24:03 +02:00

feat(pr-ci): migrate the changelog-body-check gate to rt (#607 gate 3 of 4)

shipwright referenced this pull request 2026-08-17 18:24:32 +02:00

feat(pr-ci): migrate the changelog-body-check gate to rt (#607 gate 3 of 4) #676

shipwright referenced this pull request 2026-08-17 18:43:00 +02:00

feat(pr-ci-gates): migrate PR-CI validators from bash to rt (single-stack v1.0.0) #607

Sign in to join this conversation.

Reviewers

No reviewers

lookout

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

3 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!664

No description provided.