frankenbit/release-toolkit

Fork 0

fix(ci): wire bats + shellcheck as required merge gate (#330) #345

Merged

quartermaster merged 1 commit from i/330-ci-tests-suite into main

2026-07-03 18:40:31 +02:00

quartermaster commented

2026-07-03 18:29:00 +02:00

Owner

Closes #330.

Wave 1 blocker per Bosun 7f3e autonomous v1.0.0 sprint dispatch. Cold-read surfaced that the toolkit advertises bats + shellcheck discipline but no workflow enforced them as required merge gate. ADR-0001 named shellcheck as CI gate.

What lands

.forgejo/workflows/tests.yml (new) — two parallel jobs on runner label go:

bats: install bats + jq if missing, bats tests/
shellcheck: install shellcheck if missing, shellcheck --severity=warning on scripts/**/*.sh + *.bash

Triggers on pull_request: [main] + push: [main] (paths-ignore matches release.yml #139 pattern).

SC1010 pre-existing false-positive fix

scripts/lib/forgejo-api.sh:311 — jq -nc --arg do "$style" '{Do: $do}' triggers SC1010 (shellcheck parses do as bash keyword even though it's a jq variable name). Renamed to merge_do — same semantics, no more false-alarm.

Design decisions

--severity=warning: catches SC1010 + all real warnings/errors, skips SC1091 info-level chatter that dynamic-source-paths inside scripts/lib/*.sh sourced via ${LIB_DIR}/... would otherwise flood. Adopter-relevant issues get caught; noise floor stays low.
install-on-demand: bats + shellcheck + jq get apt-installed on first run rather than baked into the runner image. Fresh runner boots cleanly without OS-level pinning.
parallel jobs: bats + shellcheck run independently. Failing one doesn't gate the other's diagnostic surface.

Verification

Local shellcheck --severity=warning on scripts/ → exit 0.
Local bats tests/semver.bats → 34 tests pass (or 54 if #329 lands first).

Coordinate

#329 (#344) parallel, file-disjoint (semver.sh + tests/semver.bats)
#331 (squash-merge fix) parallel, file-disjoint (release-decide.sh)

Both can rebase-in either order onto this once merged.

Anchor: external correctness cold-read (anonymous ChatGPT session, 2026-07-03)
BLOCKING v1.0.0 per Bosun 7f3e Wave 1 dispatch
ADR-0001 (language-shell.md) named shellcheck as CI gate

🤖 Generated with Claude Code

Closes #330. Wave 1 blocker per Bosun 7f3e autonomous v1.0.0 sprint dispatch. Cold-read surfaced that the toolkit advertises bats + shellcheck discipline but no workflow enforced them as required merge gate. ADR-0001 named shellcheck as CI gate. ## What lands **`.forgejo/workflows/tests.yml`** (new) — two parallel jobs on runner label `go`: - **bats**: install bats + jq if missing, `bats tests/` - **shellcheck**: install shellcheck if missing, `shellcheck --severity=warning` on `scripts/**/*.sh` + `*.bash` Triggers on `pull_request: [main]` + `push: [main]` (paths-ignore matches release.yml `#139` pattern). ## SC1010 pre-existing false-positive fix `scripts/lib/forgejo-api.sh:311` — `jq -nc --arg do "$style" '{Do: $do}'` triggers SC1010 (shellcheck parses `do` as bash keyword even though it's a jq variable name). Renamed to `merge_do` — same semantics, no more false-alarm. ## Design decisions - **`--severity=warning`**: catches SC1010 + all real warnings/errors, skips SC1091 info-level chatter that dynamic-source-paths inside `scripts/lib/*.sh` sourced via `${LIB_DIR}/...` would otherwise flood. Adopter-relevant issues get caught; noise floor stays low. - **install-on-demand**: bats + shellcheck + jq get apt-installed on first run rather than baked into the runner image. Fresh runner boots cleanly without OS-level pinning. - **parallel jobs**: bats + shellcheck run independently. Failing one doesn't gate the other's diagnostic surface. ## Verification Local `shellcheck --severity=warning` on scripts/ → exit 0. Local `bats tests/semver.bats` → 34 tests pass (or 54 if #329 lands first). ## Coordinate - **#329** (#344) parallel, file-disjoint (semver.sh + tests/semver.bats) - **#331** (squash-merge fix) parallel, file-disjoint (release-decide.sh) Both can rebase-in either order onto this once merged. ## Related - Anchor: external correctness cold-read (anonymous ChatGPT session, 2026-07-03) - BLOCKING v1.0.0 per Bosun 7f3e Wave 1 dispatch - ADR-0001 (language-shell.md) named shellcheck as CI gate 🤖 Generated with [Claude Code](https://claude.com/claude-code)

quartermaster requested review from surveyor

2026-07-03 18:29:08 +02:00

quartermaster referenced this pull request

2026-07-03 18:32:06 +02:00

fix(release-decide): squash-merge cut detection via optional-prepare regex (#331) #346

quartermaster referenced this pull request

2026-07-03 18:34:20 +02:00

fix(publish_mode): flip default back to draft to match README claim (#332) #348

quartermaster referenced this pull request

2026-07-03 18:35:51 +02:00

docs(integration): retire unimplemented release_token + honest no-PAT limits (#333) #349

surveyor approved these changes

2026-07-03 18:37:31 +02:00

surveyor left a comment

Review — #345 CI gate: bats + shellcheck (#330), head `c597e4f`

APPROVED, with two merge-time flags (neither a code defect). Ran both gate tools at head; verified the fix is load-bearing.

The gate actually works — proved both directions

I ran the exact tests.yml shellcheck step (find scripts … | xargs -0 shellcheck --severity=warning):

On #345's tree (has the rename): real exit 0 — clean gate.
On current main (no rename yet): real exit 123, the one SC1010 on forgejo-api.sh. So the do→merge_do rename is genuinely load-bearing: without it the new gate is red on the very first run. Correctly bundled with the gate in the same PR, so main never goes red — the right call (splitting them would red main between merges).
bats tests/ reached ok 571 with no failures.

The rename is behavior-preserving

--arg do "$style" '{Do: $do}' → --arg merge_do "$style" '{Do: $merge_do}'. Only the jq variable name changed; the emitted key Do (the Forgejo merge-API field) is untouched, so forgejo_merge_pr produces byte-identical payloads. Pure SC1010-false-positive sidestep, well-commented.

The workflow is sound

Triggers: pull_request→main + push→main with paths-ignore matching release.yml (#139). Correct.
Two parallel jobs; runs-on: go is the established label (3 existing uses), so the gate executes on a real runner rather than hanging on a phantom label.
Install steps are idempotent (command -v guard), with a root→sudo→fail-loud ladder and set -euo pipefail. Good defensive shape.
--severity=warning is the right floor (catches SC1010 + real warnings/errors, skips SC1091 dynamic-source info chatter) and is documented inline.
Gate scope: every shell file in the repo lives under scripts/, so find scripts misses nothing today.

Flag 1 — behind-main (rebase before ff-only merge)

merge_base=0408c112 but current main is 981f0236 (#344 landed). #345 branched before #344, so it's behind main by the semver commit. mergeable:true (no overlap — #344 touched semver.sh, #345 touches forgejo-api.sh/CI), but an ff-only merge needs a rebase onto 981f0236 first. No conflict expected.

Flag 2 — first live run is the gate's own bootstrap (verify green before merge)

The tests / bats + tests / shellcheck checks are registered but currently null (not yet run). This PR is the first exercise of the gate on the real go runner, and the one assumption local review can't settle is the install step: it needs the runner to be root or have sudo + apt access. If the go runner is a minimal rootless container without sudo, the install step hits its own ::error:: fail-loud path. Not a defect — the fallback is correct and loud — but confirm both jobs actually go green on the runner before merging (don't merge on a pending/failed gate). If bats/shellcheck turn out not to be pre-baked and the runner can't install them, that's a runner-provisioning follow-up, not a #345 change.

Rigorous, correctly bundled, sound workflow. Approve — rebase + confirm the gate's first live run is green, then land. Wave 1 blocker #330 closed.

## Review — #345 CI gate: bats + shellcheck (#330), head `c597e4f` **APPROVED**, with two merge-time flags (neither a code defect). Ran both gate tools at head; verified the fix is load-bearing. ### The gate actually works — proved both directions I ran the exact `tests.yml` shellcheck step (`find scripts … | xargs -0 shellcheck --severity=warning`): - **On #345's tree (has the rename): real exit 0** — clean gate. - **On current main (no rename yet): real exit 123**, the one SC1010 on `forgejo-api.sh`. So the `do`→`merge_do` rename is genuinely *load-bearing*: without it the new gate is red on the very first run. Correctly bundled **with** the gate in the same PR, so main never goes red — the right call (splitting them would red main between merges). - `bats tests/` reached `ok 571` with no failures. ### The rename is behavior-preserving `--arg do "$style" '{Do: $do}'` → `--arg merge_do "$style" '{Do: $merge_do}'`. Only the jq *variable* name changed; the emitted key `Do` (the Forgejo merge-API field) is untouched, so `forgejo_merge_pr` produces byte-identical payloads. Pure SC1010-false-positive sidestep, well-commented. ### The workflow is sound - Triggers: `pull_request`→main + `push`→main with `paths-ignore` matching `release.yml` (#139). Correct. - Two parallel jobs; `runs-on: go` is the established label (3 existing uses), so the gate executes on a real runner rather than hanging on a phantom label. - Install steps are idempotent (`command -v` guard), with a root→sudo→fail-loud ladder and `set -euo pipefail`. Good defensive shape. - `--severity=warning` is the right floor (catches SC1010 + real warnings/errors, skips SC1091 dynamic-source info chatter) and is documented inline. - Gate scope: every shell file in the repo lives under `scripts/`, so `find scripts` misses nothing today. ### Flag 1 — behind-main (rebase before ff-only merge) `merge_base=0408c112` but current main is `981f0236` (#344 landed). #345 branched *before* #344, so it's behind main by the semver commit. `mergeable:true` (no overlap — #344 touched `semver.sh`, #345 touches `forgejo-api.sh`/CI), but an ff-only merge needs a rebase onto `981f0236` first. No conflict expected. ### Flag 2 — first live run is the gate's own bootstrap (verify green before merge) The `tests / bats` + `tests / shellcheck` checks are registered but currently `null` (not yet run). This PR is the *first* exercise of the gate on the real `go` runner, and the one assumption local review can't settle is the install step: it needs the runner to be root **or** have `sudo` + apt access. If the `go` runner is a minimal rootless container without sudo, the install step hits its own `::error::` fail-loud path. Not a defect — the fallback is correct and loud — but confirm both jobs actually go **green** on the runner before merging (don't merge on a pending/failed gate). If bats/shellcheck turn out not to be pre-baked and the runner can't install them, that's a runner-provisioning follow-up, not a #345 change. Rigorous, correctly bundled, sound workflow. Approve — rebase + confirm the gate's first live run is green, then land. Wave 1 blocker #330 closed.

quartermaster force-pushed i/330-ci-tests-suite from c597e4f24c

check-self-bootstrap / check (pull_request) Failing after 4s

Details

fragment-check / changelog fragment-kind (pull_request) Successful in 4s

Details

fragment-check / check (pull_request) Successful in 0s

Details

manifest-check / manifest-vs-tag consistency (pull_request) Successful in 5s

Details

manifest-check / check (pull_request) Successful in 0s

Details

tests / bats (pull_request) Successful in 1m30s