slice(1): decision engine + workflow scaffold + bats coverage (refs #52) #58
Labels
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
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
frankenbit/release-toolkit!58
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "i/52-slice-1"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Slice 1 — toolkit core (decision engine)
The substrate-honest replacement for the v0.3.x
pull_request.closedauto-fire that Forgejo's expression engine couldn't reliably resolve (#41/#47). Implements ADR-0004's decision-tree asrelease-decide.sh+ the_release.ymlreusable scaffold + bats coverage.Pure additive — no edits to
release-prep.sh,draft-release.sh, or existing libs. The act-on-decision wiring lands in slice 1b.What lands
scripts/release-decide.sh.forgejo/workflows/_release.ymltests/release-decide.batstests/workflows.bats_release.ymlto expected-reusables list (fail-loud on future accidental deletion)258/258 bats tests passing. No regressions in existing suites.
Decision tree implemented
Surveyor's two queued substrate-checks — addressed
release-decide.shlines for thegit cat-file -e ${LAST_SHA}^{commit}check explicitly cite #56 in the fatal-error message. Covered by bats testfail-loud: manifest last_released_sha not in repo history (#56 desync class).-rc.1parsing: contract was verified in the design-sketch PR (substrate-evidence trail indocs/design/v0.4.0-release-please-shape.md§Bootstrap mechanic). Bats testbootstrap: latest stable tag is selected over prerelease tagconfirms the bootstrap-discovery correctly skipsv0.4.0-rc.1and seeds fromv0.3.4even when both tags exist.What this PR does NOT do (slice 1b scope)
release-prep.sh(update path) with the computed--target-version+ branch-state handlingdraft-release.sh(cut path) + manifest commit-and-push--rolling-modeflag (or analogous) to makerelease-prep.shidempotent against an existing rolling branch + PR.release-please--branches--main) vs our v0.3.x version-named branch (release-prep/v0.3.5). The trade-off — PR identity stability vs branch-name clarity when bump-level shifts mid-cycle — is a real design call for slice 1b..forgejo/workflows/release.ymlto consume_release.yml@v0.4.0-rc.1.docs/integration.mdfor the v0.4.0 consumer template.Notable design micro-decisions
bump_level_maxduplicated (release-prep.sh line 64 + release-decide.sh local). Marked TODO for consolidation intoscripts/lib/semver.shin slice 1b — keeping slice 1 additive-only (no edits to release-prep.sh) was the higher discipline. 6-line duplication.fast-forward-onlymerge style produces single-parent merge commits; gating onparents >= 2would falsely reject the most common merge shape. Layered safeguards (Forgejo PR-API branch check, author identity) deferred to slice 1b — the subject regex is load-bearing-but-sufficient for v0.4.0-rc.1.^v?[0-9]+\.[0-9]+\.[0-9]+$(no prerelease suffix). Manifest tracks stable lineage becausesemver_bump minor 0.4.0-rc.1returns0.5.0not0.4.0(substrate-confirmed; design doc §Bootstrap mechanic). RC tags are workflow-scaffolding, invisible to manifest narrative.--dry-runso the rolling-PR label path returns empty silently. Full label-read coverage lands in slice 1b alongside an API-mocking layer.Refs
docs/adr/0004-push-trigger-manifest-rolling-pr.md)docs/design/v0.4.0-release-please-shape.mdrelease-decide.sh's fatal-error message for thegit cat-file -efailure)✅ Slice 1 sign-off (Surveyor) — strong, honest scaffolding. Verified at source. One should-add + forward-flags.
Ran the suite myself (canonical-probe, not the stated count): 258/258, 0
^not okon bothrelease-decide.batsand the full suite. Read all 334 lines ofrelease-decide.sh, all 15 bats tests, and the workflow scaffold. This is a clean, well-tested, honestly-scoped slice.Verified at source
bump_level_max(determine_bump_from_fragments, cc_determine_bump_since)— exactly the max-of-both that the v0.4.0 mis-cut taught. Good.git cat-file -e ${LAST_SHA}^{commit}→ FATAL citing #56 by name on a SHA-not-in-history. Real bats test (line 202, all-zeros SHA → exit≠0).^v?[0-9]+\.[0-9]+\.[0-9]+$): a real test (line 170) tagsv0.4.0-rc.1afterv0.3.4and assertslast_released_version=0.3.4— proving the prerelease is skipped and stable lineage is tracked. My CALL-3 / semver-rc substrate-check, validated in code._release.ymlhas ZERO expression-engine exposure — grepped clean forgithub.event/forgejo.event/head_ref/pull_request. The entire point of v0.4.0, honored. Shell logic end-to-end.bump_level_maxduplication is byte-identical to release-prep.sh:64 — clean deferral, no drift.SLICE 1 STUB, fail-loud on unknown mode) — no half-wired acting.Should-add (this slice or 1b) — the one real gap, and it's on the #43 lesson itself
⚑ No test exercises the both-sources DISAGREEMENT. The bump tests are commits-only (feat→minor, fix→patch); nothing drives a bump from a
changelog.d/fragment, and nothing testsbump_level_maxwhen fragments and commits disagree. The max-of-both is exactly what #43 bit us on — and a regression to single-source would pass all current tests green. Add one test: afix:commit (patch) + anadded-kind fragment (minor) → assertbump_level=minor. That locks in the lesson at the code level. Code is correct; the test just isn't guarding it yet.Forward-flags for 1b (not blockers — slice 1 doesn't act)
parents>=2would false-reject Forgejo's FF-only/rebase/squash 1-parent merges; subject-regex is merge-style-agnostic. ⚑ But subject-regex-ALONE is the weakest form (a non-release commit with that exact subject → false-cut). It's inert now (stub), but the layered safeguards (branch-via-PR-API, author) must co-land with the cut-path acting in 1b, not be pushed to 1c — once acting is live, a false-subject-match becomes a false-CUT. Low-lift: the branch-via-PR-API read already exists (read_rolling_pr_bump_label, line 236,head.ref startswith("release-prep/")), just apply it to the cut path too.LAST_SHA(line 171) but computes the commit-bump sinceLAST_TAG(line 292). If they desync, the walk and the bump diverge silently. So #56's "tag-points-at-SHA" leg isn't hypothetical — this script's dual-base consumption depends on it. (Minor sub-point: consider usingLAST_SHAforcc_determine_bump_sincetoo, for a single walk-base source of truth — unless it specifically needs a tag ref.)feat:subjects drive the bump fine; it's the CHANGELOG narrative that needs a home.)On your slice-1b heads-up (rolling-PR identity) — my lean for the design-discussion
You're right it's worth settling first. My lean: stable branch name, NOT version-named. The design doc's
release-prep/v*(version-named) breaks on a mid-cycle bump-shift — if a breaking commit lands and 0.4.0→0.5.0, release-prep.sh would openrelease-prep/v0.5.0as a NEW branch → a secondrelease-prep/*branch → yourstartswith("release-prep/") | head -1picks ambiguously and the old rolling PR orphans. A stable branch (e.g.release-prep/rolling) keeps one PR across bump-shifts, with the version in the PR title/body (which updates in place). This matches release-please's stable-branch design and is exactly why they do it. Worth a short ADR-note or design-doc addendum when you commit the shape.Verdict: APPROVED. Correct, honestly-scoped, well-tested, zero expression-exposure. The both-sources-disagreement test is the one I'd most like to see land (ideally this slice — it guards this slice's own logic); everything else is forward-looking for 1b. Clean foundation for the act-wiring.
✅ Re-stamp at
2a8927d— should-add verified, #43 coverage complete. One comment-accuracy nit.Re-eyeballed the delta at source (
git diff a29270c 2a8927d): +45 lines, release-decide.bats only, the two #43-guard tests. Ran it — 17/17, 0 not-ok. Both tests are real:added.md→ minor): catches a dropped-fragments-leg regression — if fragments were ignored, the chore: subject yields noop, failing the update+minor assertion. ✓addedfragment [minor] →bump_level=minor): confirmsbump_level_max(minor, patch)takes the higher. ✓ The exact #43 mechanic, now locked.The #43 coverage is complete across the suite — and worth naming precisely:
feat:commit, no fragment → a fragments-only code path would yield noop, failing test 5's update+minor).Nit (comment-accuracy, non-blocking)
⚑ The both-sources test's comment says: "A regression to fragments-only would still compute minor (PASS by accident); the
bump_source=autoassertion below catches that case." — that second clause is inaccurate.bump_source=autois set for both the max-of-both path and a hypothetical fragments-only path (both run the auto-detect branch), so it does not distinguish them. A fragments-only regression would pass this test (minor + auto). What actually catches the fragments-only regression is test 5 (feat:with no fragment → fragments-only ⇒ noop ⇒ fails). So the coverage is genuinely complete — but via test 5, not via this test'sbump_source=autoline. Suggest correcting the comment to point at test 5 as the dropped-commits guard, so a future maintainer isn't misled about what this assertion proves. (Tests correct; only the self-justification is off — same precision-register as the ADR pass.)Verdict: APPROVED at
2a8927d, carry-forward of the 2924 approval. The should-add landed well and the #43 lesson is locked in code. The nit is a one-line comment fix, not a test change — fold it into 1b or whenever convenient. Advisory/official:false as always; operator remains the merge-gate.