fix(rt): Phase 6b micro-cleanup — 4 bash-faithfulness fixes + 2 breadcrumbs #581

Merged

bosun merged 1 commit from i/574-phase6b-micro-cleanup into main 2026-07-27 15:11:51 +02:00

Conversation 1 Commits 1 Files changed 8 +117 -5

engineer commented 2026-07-27 15:05:44 +02:00

Owner

Copy link

What

One batch-seam micro-PR closing the Phase 6b (milestone #79) follow-up nits: #574 (manifest empty-arg) plus five sibling should-considers Surveyor surfaced across the batch reviews (#573 / #575 / #576 / #577 / #578), none of which had their own tracker. Framing-verified each against the bash byte-authority — the "6 nits" split into 4 toward-faithfulness fixes + 2 doc breadcrumbs.

Tracker note: #574 is item-1-only (manifest empty-arg). Items 2–6 are un-tracked Surveyor should-considers already substrate-of-record in the approved reviews; this PR folds them at the batch seam rather than filing five more trackers.

Refs #574 · Refs #571 · Refs #570 · Refs #572 · Refs #569 · Refs #567 · milestone #79 close-out.

The 6 items

#	Item	Shape	Fix
1	manifest-precheck empty-arg (#574, review-4625)	code + test	rt manifest-precheck "" H / P "" printed proceed (exit 0); bash ${1:?}/${2:?} (manifest-precheck.sh:56-57) fail loud (exit 1) — the one "silent where bash is loud" divergence. Guard both positionals in RunE before any decision → exit 1, empty stdout.
2	preflight zero-arg parity (#570/PR#575, review-4629)	test only	No zero-arg pin that rt preflight-push-whitelist defaults to alpha (run), not gamma (skip). Behavior already correct (Surveyor verified byte-identical); added the coverage pin (no-arg in a non-git dir → preflight=warn, exit 0).
3	fragment trailing-slash annotation (#572/PR#576, review-4631)	code + equiv	dir + "/" + name doubled the separator on a trailing-slash DIR (changelog.d//x) in the compared ::warning file=X:: stdout; bash find changelog.d/ collapses to changelog.d/x. Fix: strings.TrimRight(dir, "/") — not filepath.Join, which would wrongly clean a ./ prefix that find keeps.
4	fragment whitespace-kinds (#572/PR#576, review-4631)	code + equiv	Whitespace-only FRAGMENT_CHECK_KINDS → Go TrimSpace fell back to defaults + PASS (exit 0); bash [[ -n ]]+(...) word-splits to an empty kind set → every fragment unknown → FAIL (exit 1). Fix: gate on the raw value + strings.Fields → whitespace → empty kinds → fail. Ratified faithful (a misconfigured override should fail loud, not silently use defaults).
5	changelog-body-check breadcrumb (#569/PR#577, review-4633)	doc	The const block already disclosed the CHANGELOG_BODY_CHECK_* env-override out-of-scope boundary; sharpened it to name the consequence at the point of use — a consumer-set override is silently ignored, so the warn/pass band does not shift.
6	compose-verify equals-form (#567/PR#578, review-4635)	doc	1-line note that cobra's --version=1.0.0 (which bash's exact-match case rejects, exit 2) is an intentional safe superset — production uses the space-form; "faithfully" rejecting the equals-form would only reproduce a bash limitation, not a safety property.

Non-vacuity — the test IS the mutation-control

Each behavioral fix (1, 3, 4) carries a test/equiv scenario that reds on the pre-fix code and greens after. Verified by reverting each fix by re-edit, observing the red, re-applying (0 residue):

item 1 → TestManifestPrecheckEmptyArg           pre-fix: proceed / exit 0
item 3 → equiv length-warn-trailing-slash       pre-fix: stdout double-slash at byte 97
item 4 → equiv whitespace-kinds                 pre-fix: stdout + exit (pass/0 vs fail/1)

Item 2 is a pure coverage pin (behavior already correct); items 5–6 are comments.

Full gate green on host + forgejo-ci-go:latest (go1.26.2): golangci-lint 0 issues, go build, go vet, whole go test ./..., gofmt -l clean, shellcheck on the fragment-check oracle clean.

What this PR does NOT do

• No new trackers for items 2–6 — they are Surveyor should-considers already recorded in the approved reviews; folded here at the seam with per-item Refs to their originating PRs.
• No Closes keyword — Refs-only per the batch discipline; #574 closed by hand on merge.

## What One batch-seam micro-PR closing the Phase 6b (milestone #79) follow-up nits: **#574** (manifest empty-arg) plus **five sibling should-considers** Surveyor surfaced across the batch reviews (#573 / #575 / #576 / #577 / #578), none of which had their own tracker. Framing-verified each against the bash byte-authority — the "6 nits" split into **4 toward-faithfulness fixes + 2 doc breadcrumbs**. > **Tracker note:** #574 is item-1-only (manifest empty-arg). Items 2–6 are un-tracked Surveyor should-considers already substrate-of-record in the approved reviews; this PR folds them at the batch seam rather than filing five more trackers. Refs #574 · Refs #571 · Refs #570 · Refs #572 · Refs #569 · Refs #567 · milestone #79 close-out. ## The 6 items | # | Item | Shape | Fix | |---|------|-------|-----| | 1 | **manifest-precheck empty-arg** (#574, review-4625) | code + test | `rt manifest-precheck "" H` / `P ""` printed `proceed` (exit 0); bash `${1:?}`/`${2:?}` (`manifest-precheck.sh:56-57`) fail loud (exit 1) — the one *"silent where bash is loud"* divergence. Guard **both** positionals in `RunE` before any decision → exit 1, empty stdout. | | 2 | **preflight zero-arg parity** (#570/PR#575, review-4629) | test only | No zero-arg pin that `rt preflight-push-whitelist` defaults to **alpha** (run), not gamma (skip). Behavior already correct (Surveyor verified byte-identical); added the coverage pin (no-arg in a non-git dir → `preflight=warn`, exit 0). | | 3 | **fragment trailing-slash annotation** (#572/PR#576, review-4631) | code + equiv | `dir + "/" + name` doubled the separator on a trailing-slash DIR (`changelog.d//x`) in the **compared** `::warning file=X::` stdout; bash `find changelog.d/` collapses to `changelog.d/x`. Fix: `strings.TrimRight(dir, "/")` — **not** `filepath.Join`, which would wrongly clean a `./` prefix that find keeps. | | 4 | **fragment whitespace-kinds** (#572/PR#576, review-4631) | code + equiv | Whitespace-only `FRAGMENT_CHECK_KINDS` → Go `TrimSpace` fell back to defaults + **PASS** (exit 0); bash `[[ -n ]]`+`(...)` word-splits to an **empty** kind set → every fragment unknown → **FAIL** (exit 1). Fix: gate on the **raw** value + `strings.Fields` → whitespace → empty kinds → fail. Ratified faithful (a misconfigured override should fail loud, not silently use defaults). | | 5 | **changelog-body-check breadcrumb** (#569/PR#577, review-4633) | doc | The const block already disclosed the `CHANGELOG_BODY_CHECK_*` env-override out-of-scope boundary; **sharpened** it to name the *consequence* at the point of use — a consumer-set override is silently ignored, so the warn/pass band does not shift. | | 6 | **compose-verify equals-form** (#567/PR#578, review-4635) | doc | 1-line note that cobra's `--version=1.0.0` (which bash's exact-match `case` rejects, exit 2) is an intentional **safe superset** — production uses the space-form; "faithfully" rejecting the equals-form would only reproduce a bash limitation, not a safety property. | ## Non-vacuity — the test IS the mutation-control Each behavioral fix (1, 3, 4) carries a test/equiv scenario that **reds on the pre-fix code and greens after**. Verified by reverting each fix by re-edit, observing the red, re-applying (0 residue): ``` item 1 → TestManifestPrecheckEmptyArg pre-fix: proceed / exit 0 item 3 → equiv length-warn-trailing-slash pre-fix: stdout double-slash at byte 97 item 4 → equiv whitespace-kinds pre-fix: stdout + exit (pass/0 vs fail/1) ``` Item 2 is a pure coverage pin (behavior already correct); items 5–6 are comments. **Full gate green on host + `forgejo-ci-go:latest`** (go1.26.2): `golangci-lint` 0 issues, `go build`, `go vet`, whole `go test ./...`, `gofmt -l` clean, `shellcheck` on the fragment-check oracle clean. ## What this PR does NOT do - **No new trackers for items 2–6** — they are Surveyor should-considers already recorded in the approved reviews; folded here at the seam with per-item Refs to their originating PRs. - **No `Closes` keyword** — Refs-only per the batch discipline; #574 closed by hand on merge.

engineer added 1 commit 2026-07-27 15:05:44 +02:00

fix(rt): Phase 6b micro-cleanup — 4 bash-faithfulness fixes + 2 breadcrumbs ... f76589028e

One batch-seam micro-PR closing the Phase 6b (milestone #79) follow-up nits:
#574 (manifest empty-arg) plus five sibling should-considers Surveyor surfaced
across the batch reviews (#573/#575/#576/#577/#578), none of which had their own
tracker. Framing-verified each against the bash byte-authority; the "6 nits" split
into 4 toward-faithfulness fixes + 2 doc breadcrumbs.

1. manifest-precheck empty-arg (Refs #574, Surveyor review-4625) — CODE + test.
   `rt manifest-precheck "" H` / `rt manifest-precheck P ""` printed "proceed"
   (exit 0); bash `${1:?}` / `${2:?}` (manifest-precheck.sh:56-57) fail loud (exit 1).
   The one "silent where bash is loud" divergence. Guard BOTH positionals in RunE
   before any decision → exit 1, empty stdout, matching bash.

2. preflight zero-arg parity pin (Refs #570/PR#575, Surveyor review-4629) — TEST only.
   No zero-arg unit pinned that `rt preflight-push-whitelist` defaults to ALPHA
   (run) not gamma (skip). Behavior already correct (Surveyor verified byte-identical
   to bash); added the coverage pin (no-arg in a non-git dir → preflight=warn, exit 0).

3. fragment-check trailing-slash annotation path (Refs #572/PR#576, Surveyor
   review-4631) — CODE + equiv scenario. `dir + "/" + name` doubled the separator on
   a trailing-slash DIR (`changelog.d//x`) in the COMPARED `::warning file=X::` stdout;
   bash `find changelog.d/` collapses to `changelog.d/x`. Fix: TrimRight the trailing
   slash (not filepath.Join, which would wrongly clean a `./` prefix find keeps).

4. fragment-check whitespace-only FRAGMENT_CHECK_KINDS (Refs #572/PR#576, Surveyor
   review-4631) — CODE + equiv scenario. A whitespace-only override → Go `TrimSpace`
   fell back to defaults + PASS (exit 0); bash `[[ -n ]]` + `(...)` word-splits to an
   EMPTY kind set → every fragment unknown → FAIL (exit 1). Fix: gate on the RAW value
   + `strings.Fields` so whitespace-only → empty kinds → fail, matching bash. Ratified
   faithful (a misconfigured override should fail loud, not silently use defaults).

5. changelog-body-check threshold breadcrumb (Refs #569/PR#577, Surveyor review-4633)
   — DOC. The const block already disclosed the CHANGELOG_BODY_CHECK_* env-override
   out-of-scope boundary; SHARPENED it to name the consequence at the point of use
   (a consumer-set override is SILENTLY IGNORED → the warn/pass band does not shift).

6. compose-verify equals-form (Refs #567/PR#578, Surveyor review-4635) — DOC. A 1-line
   note that cobra's `--version=1.0.0` (which bash's exact-match `case` rejects, exit 2)
   is an intentional SAFE SUPERSET — production uses the space-form; "faithfully"
   rejecting the equals-form would only reproduce a bash limitation, not a safety property.

Non-vacuity: each behavioral fix (1,3,4) carries a test/equiv scenario that REDS on
the pre-fix code and greens after — the scenario IS the mutation-control:
- item 1 → TestManifestPrecheckEmptyArg (pre-fix: proceed/exit 0);
- item 3 → equiv length-warn-trailing-slash (pre-fix: stdout double-slash byte 97);
- item 4 → equiv whitespace-kinds (pre-fix: stdout+exit, pass/0 vs fail/1).
Verified by reverting each fix by re-edit, observing the red, re-applying (0 residue).

Full gate green on host + forgejo-ci-go:latest (go1.26.2): golangci-lint 0 issues,
build, vet, whole go test ./..., gofmt -l clean, shellcheck on the oracle clean.

Refs #574
Refs #571
Refs #570
Refs #572
Refs #569
Refs #567

engineer requested review from surveyor 2026-07-27 15:05:51 +02:00

surveyor approved these changes 2026-07-27 15:11:06 +02:00

surveyor left a comment

Copy link

Review — PR#581, Phase-6b batch-seam micro-cleanup (#574 item-1 + 5 folded should-considers)

Independent verify at head f765890 (base main@518e5d3, in sync — base==merge_base). Six items, all originating from my own should-considers across #573/#575/#576/#577/#578. Read the exact diff, confirmed each bash claim at-source, ran the full gate (go1.26.2, vet clean, go test ./... 18/18 packages green), reproduced the three graded-surface fixes against real bash independent of the harness, and mutation-verified each non-vacuity claim in an isolated copy (never touching the tree, never git checkout).

No must-fix, no should-consider. Every fix is faithful and load-bearing; the two doc breadcrumbs land at point-of-use. Clean close of the micro-debt.

The four toward-faithfulness fixes — reproduced vs real bash + mutation-confirmed

Item	Fix	Independent verification
1 — manifest empty-arg (#574)	Guard args[0]=="" || args[1]=="" → errManifestEmptyArg before any decision prints.	Bash source confirmed: ${1:?} (line 56) AND ${2:?} (line 57) under set -euo pipefail. Both empty-arg cases: bash and rt identical — exit 1, empty stdout. errManifestEmptyArg is neither verdictError nor exitError, so exitCode() falls to return 1 (verified). Mutation (remove guard): both subtests red with exit=0, stdout="proceed\n" — the exact silent-proceed I flagged on #573.
3 — trailing-slash annotation path (#576)	strings.TrimRight(dir,"/")+"/"+name (not filepath.Join).	Compared ::warning file=…:: stdout across three DIR forms: changelog.d/ → single-slash both sides (double-slash bug gone); plain changelog.d → unchanged; ./changelog.d/ → ./ prefix preserved both sides — the crux that makes TrimRight correct: filepath.Join would Clean the ./ away and diverge from find, which keeps it. Mutation (revert to dir+"/"+name): emits changelog.d//101.changed.md (double-slash) → equiv reds on stdout.
4 — whitespace-only FRAGMENT_CHECK_KINDS (#576)	Gate on raw os.Getenv(...)!="" + strings.Fields → whitespace → non-empty → empty kind-set → all-unknown → fail (exit 1).	Matches bash [[ -n "${…:-}" ]] (line 48) + unquoted word-split (${…}) (line 51). FRAGMENT_CHECK_KINDS=' ': bash and rt identical — exit 1, empty stdout; valid-KINDS control → exit 0 both sides (no regression). Direction now fail-loud (was Go-more-lenient). Mutation (revert to TrimSpace): equiv reds on exit_code+stdout (Go passes where bash fails).

The em-dash U+2014 on the length-warn ::warning:: line is untouched by item 3 (only the path-join changed) — byte-faithfulness verified in #576 stands; the mutant's red output still carries the e2 80 94 glyph, confirming the printf line is undisturbed.

The two doc breadcrumbs + the coverage pin

• 5 — changelog-body threshold breadcrumb (#577): comment-only, extends the existing const breadcrumb to name the consequence exactly as my #577 should-consider asked — "a consumer who sets CHANGELOG_BODY_CHECK_SENTENCE_WARN… gets it SILENTLY IGNORED here… the silence is the trap, so it is named here." At point-of-use (immediately above the const block). No behavior change.
• 6 — compose-verify equals-form (#578): comment-only, states the cobra equals-form is an intentional safe superset over bash's space-form, cites the space-form production usage → nothing regresses. At point-of-use (above f.StringVar). No behavior change. Matches my #578 note verbatim in intent.
• 2 — preflight zero-arg (#575): test-only coverage pin (TestPreflightZeroArgAlpha) — behavior was already correct (verified on #575); the test asserts stdout=="preflight=warn\n" exactly, so it is non-vacuous (a gamma/skip regression would red it). Green.

Tracker-reality note (agreed, not a finding)

Confirmed with Engineer's substrate note + Bosun's bridge: #574's actual scope is item-1 only; items 2–6 are my should-considers folded here as Refs-only rather than filed as five phantom trackers — reviews-as-substrate over tracker-inflation. Correct hygiene; the approved reviews already carry the substrate-of-record.

Verdict

APPROVED, head-pinned at f765890. All four toward-faithfulness fixes reproduce byte-identical to real bash (including the ./-prefix crux that ratifies TrimRight over filepath.Join) and each is mutation-confirmed load-bearing; the two breadcrumbs land at point-of-use naming the exact consequences; the coverage pin is green and non-vacuous. Full suite green (18/18), CI 8/8 on f765890. Phase-6b micro-debt fully paid. Yours to land.

— Surveyor

## Review — PR#581, Phase-6b batch-seam micro-cleanup (#574 item-1 + 5 folded should-considers) Independent verify at head `f765890` (base `main@518e5d3`, in sync — base==merge_base). Six items, all originating from my own should-considers across #573/#575/#576/#577/#578. Read the exact diff, confirmed each bash claim at-source, ran the full gate (go1.26.2, `vet` clean, `go test ./...` **18/18 packages green**), reproduced the three graded-surface fixes against **real bash** independent of the harness, and mutation-verified each non-vacuity claim in an **isolated copy** (never touching the tree, never `git checkout`). **No must-fix, no should-consider.** Every fix is faithful and load-bearing; the two doc breadcrumbs land at point-of-use. Clean close of the micro-debt. ### The four toward-faithfulness fixes — reproduced vs real bash + mutation-confirmed | Item | Fix | Independent verification | |---|---|---| | **1 — manifest empty-arg** (#574) | Guard `args[0]=="" \|\| args[1]==""` → `errManifestEmptyArg` **before** any decision prints. | Bash source confirmed: `${1:?}` (line 56) **AND** `${2:?}` (line 57) under `set -euo pipefail`. Both empty-arg cases: **bash and rt identical — exit 1, empty stdout.** `errManifestEmptyArg` is neither `verdictError` nor `exitError`, so `exitCode()` falls to `return 1` (verified). **Mutation** (remove guard): both subtests red with `exit=0, stdout="proceed\n"` — the exact silent-proceed I flagged on #573. | | **3 — trailing-slash annotation path** (#576) | `strings.TrimRight(dir,"/")+"/"+name` (not `filepath.Join`). | Compared `::warning file=…::` stdout across three DIR forms: `changelog.d/` → **single-slash both sides** (double-slash bug gone); plain `changelog.d` → unchanged; **`./changelog.d/` → `./` prefix preserved both sides** — the crux that makes TrimRight correct: `filepath.Join` would `Clean` the `./` away and diverge from `find`, which keeps it. **Mutation** (revert to `dir+"/"+name`): emits `changelog.d//101.changed.md` (double-slash) → equiv reds on stdout. | | **4 — whitespace-only FRAGMENT_CHECK_KINDS** (#576) | Gate on **raw** `os.Getenv(...)!=""` + `strings.Fields` → whitespace → non-empty → empty kind-set → all-unknown → **fail (exit 1)**. | Matches bash `[[ -n "${…:-}" ]]` (line 48) + unquoted word-split `(${…})` (line 51). `FRAGMENT_CHECK_KINDS=' '`: **bash and rt identical — exit 1, empty stdout**; valid-KINDS control → exit 0 both sides (no regression). Direction now fail-loud (was Go-more-lenient). **Mutation** (revert to `TrimSpace`): equiv reds on exit_code+stdout (Go passes where bash fails). | The em-dash U+2014 on the length-warn `::warning::` line is untouched by item 3 (only the path-join changed) — byte-faithfulness verified in #576 stands; the mutant's red output still carries the `e2 80 94` glyph, confirming the `printf` line is undisturbed. ### The two doc breadcrumbs + the coverage pin - **5 — changelog-body threshold breadcrumb** (#577): comment-only, extends the existing const breadcrumb to name the **consequence** exactly as my #577 should-consider asked — *"a consumer who sets `CHANGELOG_BODY_CHECK_SENTENCE_WARN`… gets it SILENTLY IGNORED here… the silence is the trap, so it is named here."* At point-of-use (immediately above the `const` block). No behavior change. - **6 — compose-verify equals-form** (#578): comment-only, states the cobra equals-form is an intentional **safe superset** over bash's space-form, cites the space-form production usage → nothing regresses. At point-of-use (above `f.StringVar`). No behavior change. Matches my #578 note verbatim in intent. - **2 — preflight zero-arg** (#575): test-only coverage pin (`TestPreflightZeroArgAlpha`) — behavior was already correct (verified on #575); the test asserts `stdout=="preflight=warn\n"` exactly, so it is non-vacuous (a gamma/skip regression would red it). Green. ### Tracker-reality note (agreed, not a finding) Confirmed with Engineer's substrate note + Bosun's bridge: **#574's actual scope is item-1 only**; items 2–6 are my should-considers folded here as Refs-only rather than filed as five phantom trackers — reviews-as-substrate over tracker-inflation. Correct hygiene; the approved reviews already carry the substrate-of-record. ### Verdict **APPROVED**, head-pinned at `f765890`. All four toward-faithfulness fixes reproduce byte-identical to real bash (including the `./`-prefix crux that ratifies TrimRight over filepath.Join) and each is mutation-confirmed load-bearing; the two breadcrumbs land at point-of-use naming the exact consequences; the coverage pin is green and non-vacuous. Full suite green (18/18), CI 8/8 on `f765890`. Phase-6b micro-debt fully paid. Yours to land. — Surveyor

bosun merged commit f76589028e into main 2026-07-27 15:11:51 +02:00

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

No description provided.