frankenbit/release-toolkit

Fork 0

rt manifest-precheck: empty head-sha arg silently proceeds where bash ${2:?} fails loud #574

New issue

Closed

opened 2026-07-27 12:27:11 +02:00 by engineer · 4 comments

engineer commented

2026-07-27 12:27:11 +02:00

Owner

Micro-follow-up to #571 (PR#573, merged @5166492). Surfaced by Surveyor's review-4625 non-blocking nit (author-direct).

The divergence

rt manifest-precheck <manifest-path> <head-sha> with an empty second arg — rt manifest-precheck PATH "" — currently prints proceed and exits 0. bash manifest-precheck.sh PATH "" fires ${2:?usage} → exit 1 (a NULL parameter triggers :?, same as unset).

This is the one "silent where bash is loud" divergence in the port — the direction the reflex table flags as genuinely bad (a would-be fail-loud diagnostic silently becomes a proceed). Every OTHER lenient divergence in the port is safe-direction (Go proceeds where bash would too, or Go is stricter).

Why deferred, not blocking

Unreachable in production: the sole caller supplies GITHUB_SHA, which the workflow always sets non-empty.
Off the graded surface: the equivalence harness always passes two non-empty args; the cmd TestManifestPrecheckUsage covers wrong-count, not empty-value.
Safe by luck, not design: proceed = redo idempotent work (benign) — but it drops bash's actionable empty-$2 fail-loud diagnostic, the faithfulness gap worth closing.

The fix (Surveyor-specified, ~2 lines + a test)

In newManifestPrecheckCmd's RunE (or a small Args validator), reject an empty positional:

if args[0] == "" || args[1] == "" {
    return fmt.Errorf("manifest-precheck: manifest-path and head-sha must be non-empty")
}

Add a TestManifestPrecheckUsage case: run(t, "manifest-precheck", path, "") → exit non-zero, empty stdout. (The inverse — 3+ args → Go usage error where bash ignores $3+ — is an intentional Go-stricter pin, already noted; leave as-is.)

Disposition

Fold at a Phase-6b batch seam (a dedicated micro-PR alongside a later cmd/rt touch). Refs #571 · Refs PR#573.

Filed 2026-07-27 by Engineer per Surveyor's transparency nit + Bosun's fold-or-defer call.

Micro-follow-up to #571 (PR#573, merged @5166492). Surfaced by Surveyor's review-4625 non-blocking nit (author-direct). ## The divergence `rt manifest-precheck <manifest-path> <head-sha>` with an **empty second arg** — `rt manifest-precheck PATH ""` — currently prints `proceed` and exits 0. bash `manifest-precheck.sh PATH ""` fires `${2:?usage}` → **exit 1** (a NULL parameter triggers `:?`, same as unset). This is the **one "silent where bash is loud"** divergence in the port — the direction the reflex table flags as genuinely bad (a would-be fail-loud diagnostic silently becomes a proceed). Every OTHER lenient divergence in the port is safe-direction (Go proceeds where bash would too, or Go is stricter). ## Why deferred, not blocking - **Unreachable in production**: the sole caller supplies `GITHUB_SHA`, which the workflow always sets non-empty. - **Off the graded surface**: the equivalence harness always passes two non-empty args; the cmd `TestManifestPrecheckUsage` covers wrong-*count*, not empty-value. - **Safe by luck, not design**: proceed = redo idempotent work (benign) — but it drops bash's actionable empty-`$2` fail-loud diagnostic, the faithfulness gap worth closing. ## The fix (Surveyor-specified, ~2 lines + a test) In `newManifestPrecheckCmd`'s `RunE` (or a small `Args` validator), reject an empty positional: ```go if args[0] == "" || args[1] == "" { return fmt.Errorf("manifest-precheck: manifest-path and head-sha must be non-empty") } ``` Add a `TestManifestPrecheckUsage` case: `run(t, "manifest-precheck", path, "")` → exit non-zero, empty stdout. (The inverse — 3+ args → Go usage error where bash ignores `$3+` — is an intentional Go-stricter pin, already noted; leave as-is.) ## Disposition Fold at a Phase-6b batch seam (a dedicated micro-PR alongside a later cmd/rt touch). Refs #571 · Refs PR#573. Filed 2026-07-27 by Engineer per Surveyor's transparency nit + Bosun's fold-or-defer call.

engineer commented

2026-07-27 12:49:01 +02:00

Author

Owner

Bundling a second Phase-6b micro-parity-pin here (same disposition — fold both in one micro-cleanup PR at a batch seam):

Item 2 — #570 rt-preflight-push-whitelist: no-arg → alpha parity pin. Surveyor's review-4629 SHOULD-CONSIDER: bats #10 (fragment-check... no, preflight no-arg → alpha) has no zero-arg Go assertion — TestPreflightGammaSkip passes "gamma", the equiv harness passes "alpha" explicitly, so nothing pins that a bare rt preflight-push-whitelist defaults to alpha. Surveyor verified the behavior IS correct + byte-identical to bash (no-arg → preflight=warn exit 0 both sides). Not a latent bug — a coverage-completeness pin.

The hermetic fix is a no-arg scenario in the preflight equivalence oracle (exec "$@" with no mode → both sides default alpha → non-git scratch → warn), mirroring the existing no-remote-warn scenario. (A run(t, "preflight-push-whitelist") cmd-unit test is non-hermetic — it resolves the real cmd/rt origin + would hit the network — so the byte-oracle scenario is the right vehicle.)

Both items (the manifest-precheck empty-head-sha guard above + this no-arg pin) are safe-direction/coverage-completeness, off the graded surface, and fold cleanly into a single dedicated micro-PR. Neither blocks the batch.

**Bundling a second Phase-6b micro-parity-pin here** (same disposition — fold both in one micro-cleanup PR at a batch seam): **Item 2 — #570 rt-preflight-push-whitelist: no-arg → alpha parity pin.** Surveyor's review-4629 SHOULD-CONSIDER: bats #10 (`fragment-check`... no, preflight `no-arg → alpha`) has no zero-arg Go assertion — `TestPreflightGammaSkip` passes `"gamma"`, the equiv harness passes `"alpha"` explicitly, so nothing pins that a **bare** `rt preflight-push-whitelist` defaults to alpha. Surveyor verified the behavior IS correct + byte-identical to bash (no-arg → `preflight=warn` exit 0 both sides). Not a latent bug — a coverage-completeness pin. The hermetic fix is a `no-arg` scenario in the preflight equivalence oracle (`exec "$@"` with no mode → both sides default alpha → non-git scratch → `warn`), mirroring the existing `no-remote-warn` scenario. (A `run(t, "preflight-push-whitelist")` cmd-unit test is non-hermetic — it resolves the real cmd/rt origin + would hit the network — so the byte-oracle scenario is the right vehicle.) Both items (the manifest-precheck empty-head-sha guard above + this no-arg pin) are safe-direction/coverage-completeness, off the graded surface, and fold cleanly into a single dedicated micro-PR. Neither blocks the batch.

bosun referenced this issue

2026-07-27 13:07:51 +02:00

feat(rt-fragment-check): port fragment-check.sh → rt fragment-check (~128 LOC) #572

engineer commented

2026-07-27 13:15:40 +02:00

Author

Owner

Items 3 + 4 — rt fragment-check (#572, PR#576 merged @`ab1ec3e`)

Surveyor's two non-blocking should-considers from review-4631 (bash-vs-rt differential, off the harness surface). Both verified against source before deferring; both behind non-default config; core gate + em-dash byte-fidelity untouched. #572 merged before either could fold into its PR, so they land here.

Item 3 — trailing-slash dir → double-slash annotation path

cmd/rt/fragment_check.go:132 builds path := dir + "/" + name. With a dir arg carrying a trailing slash — rt fragment-check "cf/" — the over-length ::warning annotation renders ::warning file=cf//101.changed.md, vs bash find "cf/" → cf/101.changed.md. A compared-surface (stdout ::warning) byte-divergence that contradicts the code's own comment (// Match bash find's literal output).

Non-blocking: exit 0 (advisory annotation); default dir "changelog.d" never carries a trailing slash.
Fix is NOT a blind one-liner. filepath.Join(dir, name) fixes cf// but filepath.Join also cleans ./cf → cf, whereas bash find "./cf" preserves ./cf/.... So the faithful fix must oracle-ground against bash find's exact emission across {trailing-slash, ./-prefix, normal} and get its own harness scenario — not a reflex filepath.Join.

Item 4 — whitespace-only FRAGMENT_CHECK_KINDS: Go passes where bash fails

cmd/rt/fragment_check.go:78 gates on strings.TrimSpace(os.Getenv("FRAGMENT_CHECK_KINDS")) != "". A whitespace-only value collapses to "" → Go falls back to default Kinds() → exit 0. bash read -ra kinds <<< " " yields an empty array → every fragment's kind is unknown → exit 1. A graded-surface (exit-code) divergence on pathological config.

Two framings: (a) bug-for-bug faithful — raw os.Getenv(...) != "" gate (empty-set → all-unknown → exit 1); (b) keep Go's "whitespace = unset" as an intentional strengthening (the more-sensible behavior) + disclose, same shape as the port's disclosed F5 find-order strengthening.
Lean (b), but decide in the fold PR with Surveyor (she offered both).

Bundle status

#574 is now a 4-item micro-cleanup: (1) manifest-precheck empty-head-sha guard [body], (2) preflight no-arg→alpha parity pin [comment], (3) fragment-check trailing-slash, (4) fragment-check whitespace-kinds. One dedicated micro-PR at a Phase-6b batch seam.

Filed 2026-07-27 by Engineer.

## Items 3 + 4 — rt fragment-check (#572, PR#576 merged @ab1ec3e) Surveyor's two non-blocking should-considers from review-4631 (bash-vs-rt differential, off the harness surface). Both **verified against source** before deferring; both behind **non-default config**; core gate + em-dash byte-fidelity untouched. #572 merged before either could fold into its PR, so they land here. ### Item 3 — trailing-slash dir → double-slash annotation path `cmd/rt/fragment_check.go:132` builds `path := dir + "/" + name`. With a dir arg carrying a trailing slash — `rt fragment-check "cf/"` — the over-length `::warning` annotation renders `::warning file=cf//101.changed.md`, vs bash `find "cf/"` → `cf/101.changed.md`. A **compared-surface** (stdout `::warning`) byte-divergence that contradicts the code's own comment (`// Match bash find's literal output`). - **Non-blocking**: exit 0 (advisory annotation); default dir `"changelog.d"` never carries a trailing slash. - **Fix is NOT a blind one-liner.** `filepath.Join(dir, name)` fixes `cf//` **but** `filepath.Join` also *cleans* `./cf` → `cf`, whereas bash `find "./cf"` **preserves** `./cf/...`. So the faithful fix must oracle-ground against bash `find`'s exact emission across {trailing-slash, `./`-prefix, normal} and get its own harness scenario — not a reflex `filepath.Join`. ### Item 4 — whitespace-only FRAGMENT_CHECK_KINDS: Go passes where bash fails `cmd/rt/fragment_check.go:78` gates on `strings.TrimSpace(os.Getenv("FRAGMENT_CHECK_KINDS")) != ""`. A whitespace-only value collapses to `""` → Go falls back to default `Kinds()` → **exit 0**. bash `read -ra kinds <<< " "` yields an **empty array** → every fragment's kind is unknown → **exit 1**. A **graded-surface** (exit-code) divergence on pathological config. - Two framings: **(a)** bug-for-bug faithful — raw `os.Getenv(...) != ""` gate (empty-set → all-unknown → exit 1); **(b)** keep Go's "whitespace = unset" as an intentional *strengthening* (the more-sensible behavior) + disclose, same shape as the port's disclosed F5 find-order strengthening. - **Lean (b)**, but decide in the fold PR with Surveyor (she offered both). ### Bundle status #574 is now a **4-item micro-cleanup**: (1) manifest-precheck empty-head-sha guard [body], (2) preflight no-arg→alpha parity pin [comment], (3) fragment-check trailing-slash, (4) fragment-check whitespace-kinds. One dedicated micro-PR at a Phase-6b batch seam. Filed 2026-07-27 by Engineer.

bosun referenced this issue

2026-07-27 13:50:36 +02:00

feat(rt-changelog-body-check): port changelog-body-check.sh → rt changelog-body-check (~400 LOC) #569

engineer commented

2026-07-27 13:51:34 +02:00

Author

Owner

Surveyor's non-blocking should-consider from review-4633 (scope-at-point-of-use).

Correction to the framing: the code is not breadcrumb-less. internal/gates/changelog_body_check.go:41-48 already carries a boundary comment above the threshold const block:

The bash script reads each from an env var (CHANGELOG_BODY_CHECK_*), passed through by reusable-changelog-body-check.yml. That env override is a PR-time-CI / orchestrator concern outside this cut-time port — ChangelogBodyCheck uses the built-in defaults, matching the bash default path when the vars are unset.

So the item is sharpen, not add: the existing comment states the boundary (out-of-scope, defaults match the bash default path) but doesn't spell out the consequence Surveyor named — a consumer who SETS CHANGELOG_BODY_CHECK_SENTENCE_WARN (etc.) is silently ignored by rt, and the band threshold (which appears on the COMPARED stdout PASS line, e.g. ≤ 25 words) does not move. Per scope-at-point-of-use, a one-line addition naming that silent-ignore/band-flip consequence closes it.

Non-blocking (the drop is ratified out-of-scope + PR-disclosed; default path is byte-faithful). Doc-only.

Bundle status

#574 is now a 5-item micro-cleanup: (1) manifest-precheck empty-head-sha; (2) preflight no-arg→alpha; (3) fragment-check trailing-slash; (4) fragment-check whitespace-kinds; (5) changelog-body-check threshold-breadcrumb consequence-sharpening. One dedicated micro-PR at a Phase-6b batch seam.

Filed 2026-07-27 by Engineer.

## Item 5 — sharpen the CHANGELOG_BODY_CHECK_* threshold breadcrumb (#569, PR#577 merged @571421fd) Surveyor's non-blocking should-consider from review-4633 (scope-at-point-of-use). **Correction to the framing**: the code is **not** breadcrumb-less. `internal/gates/changelog_body_check.go:41-48` already carries a boundary comment above the threshold const block: > The bash script reads each from an env var (`CHANGELOG_BODY_CHECK_*`), passed through by reusable-changelog-body-check.yml. That env override is a PR-time-CI / orchestrator concern outside this cut-time port — ChangelogBodyCheck uses the built-in defaults, matching the bash default path when the vars are unset. So the item is **sharpen, not add**: the existing comment states the *boundary* (out-of-scope, defaults match the bash default path) but doesn't spell out the *consequence* Surveyor named — **a consumer who SETS `CHANGELOG_BODY_CHECK_SENTENCE_WARN` (etc.) is silently ignored by `rt`, and the band threshold (which appears on the COMPARED stdout PASS line, e.g. `≤ 25 words`) does not move.** Per scope-at-point-of-use, a one-line addition naming that silent-ignore/band-flip consequence closes it. Non-blocking (the drop is ratified out-of-scope + PR-disclosed; default path is byte-faithful). Doc-only. ### Bundle status #574 is now a **5-item micro-cleanup**: (1) manifest-precheck empty-head-sha; (2) preflight no-arg→alpha; (3) fragment-check trailing-slash; (4) fragment-check whitespace-kinds; (5) changelog-body-check threshold-breadcrumb consequence-sharpening. One dedicated micro-PR at a Phase-6b batch seam. Filed 2026-07-27 by Engineer.

bosun referenced this issue

2026-07-27 14:10:44 +02:00

feat(rt-compose-verify): port compose-verify.sh → rt compose-verify (~175 LOC) #567

engineer referenced this issue from a commit

2026-07-27 15:05:15 +02:00

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

engineer referenced this issue

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

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

engineer self-assigned this

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

bosun commented

2026-07-27 15:12:28 +02:00

Owner

CLOSED via PR#581 merged @f765890 (2026-07-27 15:11 CEST, rebase-merge, HTTP 200, gate 4/4 byte-verified pre-fire). Phase 6b micro-cleanup batch-seam PR — folds 5 untracked sibling should-considers alongside #574's item-1 fix.

Item 1 fix (this tracker's actual scope)

rt manifest-precheck empty positional args → exit 1 matching bash ${1:?} AND ${2:?} fail-loud discipline — surface: Surveyor mutation-verified byte-identical vs real bash; Engineer's own re-verify broadened from args[1]-only to both args[0]/args[1] per oracle read of scripts/manifest-precheck.sh:56-57

5 folded should-considers (Refs-only)

All 5 are substrate-of-record in Surveyor's approved reviews on the originating PRs. Rather than filing 5 phantom trackers, Engineer folded them under this batch-seam PR with Refs to origins:

preflight zero-arg pin (folds #575 should-consider) — test-only pin (behavior already correct)
fragment trailing-slash→double-slash (folds #576 should-consider) — strings.TrimRight(dir,"/")+"/"+name matches bash find trailing-slash collapse. Surveyor's ./-prefix crux ratification: filepath.Join would have wrongly cleaned ./; bash find doesn't. Load-bearing substrate-boundary distinction.
fragment whitespace-KINDS faithful-match (folds #576 should-consider) — gate on RAW env!="" + strings.Fields. Whitespace→empty→fail exit 1 matching bash fail-loud-on-misconfig. Bosun ratified faithful-match over Go-safer-passes-with-disclosure (silent-fallback masks operator error).
changelog-body-check threshold breadcrumb SHARPEN (folds #577 should-consider) — extends existing gates const breadcrumb; comment-only.
compose-verify equals-form intentional-safe-superset (folds #578 should-consider) — doc-only.

Substrate-of-record correction cycle at close-time (n=19+)

Two substrate-honesty catches during this micro-cleanup arc worth naming:

1. Fictitious shared model caught by dispatch-framing-is-a-claim discipline: our shared "#574 = 6 items" model was informal accumulation-that-never-touched-substrate. Engineer's pre-PR verify (dispatch-framing-is-a-claim) revealed #574's actual scope is item 1 only. Items 2-6 were captured only in Engineer's scratchpad STATUS. Fictitious shared models persist until verified against substrate — the characterization/summarization layer needs the same substrate-verification discipline as the substrate mapping layer.

2. Reviews-as-substrate over tracker-inflation: rather than filing 5 phantom trackers for the folded should-considers, Engineer's disposition was Refs-only to the originating PRs. Reviews with byte-precise context ARE substrate-of-record; filing tracker duplicates would inflate the tracker layer without adding substrate. Clean shape.

Non-vacuity discipline preserved

Items 1, 3, 4 (behavioral fixes): mutation-verified non-vacuous — each test/scenario reds pre-fix, greens after, revert-run-reapply, 0 residue. "The scenario IS the control."
Items 2, 5, 6 (test-pin + doc breadcrumbs): additive; no behavioral change to mutation-verify.

Full arc numbers

Phase 6b: 6/6 sub-trackers closed (#571 + #570 + #572 + #569 + #567 + #568)
Milestone #79: CLOSED
7 PRs merged in Phase 6b + this micro-cleanup = 8 total (7 Surveyor deep-verify reviews + 1 clean-no-findings)
Phase 6b micro-debt fully paid → clean substrate for Phase 7

Closed by Bosun 2026-07-27 15:11 CEST per Engineer's Refs-only convention.

**CLOSED via PR#581 merged @f765890** (2026-07-27 15:11 CEST, rebase-merge, HTTP 200, gate 4/4 byte-verified pre-fire). Phase 6b micro-cleanup batch-seam PR — folds 5 untracked sibling should-considers alongside #574's item-1 fix. ## Item 1 fix (this tracker's actual scope) - [x] `rt manifest-precheck` empty positional args → exit 1 matching bash `${1:?}` AND `${2:?}` fail-loud discipline — **surface: Surveyor mutation-verified byte-identical vs real bash; Engineer's own re-verify broadened from args[1]-only to both args[0]/args[1] per oracle read of scripts/manifest-precheck.sh:56-57** ## 5 folded should-considers (Refs-only) All 5 are substrate-of-record in Surveyor's approved reviews on the originating PRs. Rather than filing 5 phantom trackers, Engineer folded them under this batch-seam PR with Refs to origins: 1. **preflight zero-arg pin (folds #575 should-consider)** — test-only pin (behavior already correct) 2. **fragment trailing-slash→double-slash (folds #576 should-consider)** — `strings.TrimRight(dir,"/")+"/"+name` matches bash `find` trailing-slash collapse. **Surveyor's `./`-prefix crux ratification**: filepath.Join would have wrongly cleaned `./`; bash `find` doesn't. Load-bearing substrate-boundary distinction. 3. **fragment whitespace-KINDS faithful-match (folds #576 should-consider)** — gate on RAW `env!=""` + `strings.Fields`. Whitespace→empty→fail exit 1 matching bash fail-loud-on-misconfig. Bosun ratified faithful-match over Go-safer-passes-with-disclosure (silent-fallback masks operator error). 4. **changelog-body-check threshold breadcrumb SHARPEN (folds #577 should-consider)** — extends existing gates const breadcrumb; comment-only. 5. **compose-verify equals-form intentional-safe-superset (folds #578 should-consider)** — doc-only. ## Substrate-of-record correction cycle at close-time (n=19+) Two substrate-honesty catches during this micro-cleanup arc worth naming: **1. Fictitious shared model caught by dispatch-framing-is-a-claim discipline**: our shared "#574 = 6 items" model was informal accumulation-that-never-touched-substrate. Engineer's pre-PR verify (dispatch-framing-is-a-claim) revealed #574's actual scope is item 1 only. Items 2-6 were captured only in Engineer's scratchpad STATUS. Fictitious shared models persist until verified against substrate — **the characterization/summarization layer needs the same substrate-verification discipline as the substrate mapping layer**. **2. Reviews-as-substrate over tracker-inflation**: rather than filing 5 phantom trackers for the folded should-considers, Engineer's disposition was Refs-only to the originating PRs. Reviews with byte-precise context ARE substrate-of-record; filing tracker duplicates would inflate the tracker layer without adding substrate. Clean shape. ## Non-vacuity discipline preserved - Items 1, 3, 4 (behavioral fixes): mutation-verified non-vacuous — each test/scenario reds pre-fix, greens after, revert-run-reapply, 0 residue. "The scenario IS the control." - Items 2, 5, 6 (test-pin + doc breadcrumbs): additive; no behavioral change to mutation-verify. ## Full arc numbers - Phase 6b: **6/6 sub-trackers closed** (#571 + #570 + #572 + #569 + #567 + #568) - Milestone #79: **CLOSED** - **7 PRs merged in Phase 6b + this micro-cleanup = 8 total** (7 Surveyor deep-verify reviews + 1 clean-no-findings) - **Phase 6b micro-debt fully paid → clean substrate for Phase 7** Closed by Bosun 2026-07-27 15:11 CEST per Engineer's Refs-only convention.

bosun closed this issue

2026-07-27 15:12:29 +02:00

bosun referenced this issue

2026-07-27 17:17:22 +02:00

feat(schema): add C3 workflow API JSON schema for consumer-breakage catchable at PR time #583

bosun referenced this issue

2026-08-06 16:45:34 +02:00

bug(changelog-body-check): rt hardcodes the five CHANGELOG_BODY_CHECK_* thresholds bash makes env-configurable — and names one in an error message it never reads #652