feat(rt-decide): port release-decide.sh → rt decide (#555) #566
No reviewers
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
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
frankenbit/release-toolkit!566
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "i/555-rt-decide"
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?
rt decide — the Go port of
scripts/release-decide.sh(#555)The last Phase-6 orchestrator port. Ports the 826-LOC decision step to a new
internal/decidepackage + a thincmd/rt/decide.gocobra wrapper.rt decidewalks git history against the manifest, queries the Forgejo PR API for the 3-layer
cut safeguard, and emits
mode=noop|cut|update(key=valuelines) to stdout.Framing (framing-verify → ratify) — NOT a Cutter consumer
The dispatch flagged this as pure-decision "but framing-verify to confirm". Confirmed
against the oracle:
release-decide.shnever tags, publishes, composes a changelog,or writes the manifest — it reads the manifest + walks
git log+ queries the PRAPI + emits a decision. So no Cutter wiring; the Cutter refinement count stays 3/5.
Three forks surfaced + ratified before building (F1/F2/F3 below).
F1 — events retired WITH REASON (milestone #78 §5)
release-decide.shcallsevent_emit cut_decided …at each mode. Those events go tothe stderr event stream (a pure observability layer), NOT the stdout the
equivalence harness grades. All four sibling orchestrators (prep/release/manifest
-check/prune-rc-tags) emit zero events at the cmd/rt layer. This port follows that
precedent and retires the emission per the milestone's pre-sanctioned
AC-retire-with-reason for the events subsystem.
internal/eventsstays byte-testedon its own; decide's compared surface is unaffected.
F2 — forgejo.Client accretion (0 Cutter, 3 forgejo additions)
The Layer-3 author check reads
.user.loginand the rolling-PR bump-label read walksopen PRs'
.labels[].name— surface the client lacked:PullRequest.Author(.user) +PullRequest.Labels([]Label{.name}) fields.ListPRs(ctx, repo, state)— the faithful port ofGET /pulls?state=open+ thecaller's
head.refstartswithrelease-prep/filter (sub-fork a). Kept ageneral list-in-a-state rather than overloading
FindPRByHeadwith labels + aprefix-match — that would silently narrow bash's startswith tolerance to exact-match
and grow a hidden second concern (scope-at-point-of-use).
FindPRByHeadstayssingle-purpose; Layer 2's
FindPRByMergeSHA+ListTagsalready existed.Additive-optional (additive-dormant): existing
Clientimplementers are unaffected(a new interface method + two tolerated-absent struct fields). Substrate growth lands
in the primitive reality pushes against — the §3.3 per-port shape (#556: 2 Cutter;
#557: 1 Cutter; #558: 1 manifest.Store; #559: 0; #555: 3 forgejo.Client).
F3 — internal/decide package (mirrors internal/prep)
826 LOC of real logic — bootstrap (#368 virtual v0.0.0), #380 tag-vs-sha divergence,
#417 orphan-CHANGELOG, the #56 desync guard, the 3-layer safeguard (subject-regex
#331/#259 + Forgejo
head.label#92 + author identity), bump-source precedence (CLI >rolling-PR label > auto) — over two injected seams: a
GitRunner(canned git; thehistory shapes are tedious to build in a scratch repo) and a
PRReader(fake forgefor the safeguard). Reuses semver/fragments/conventionalcommits/changelog/config.
Two implementer calls (no ratify needed, noted):
git rev-parse --verify) on decide'sown GitRunner and emits decide's own resolution message. It does NOT reuse
manifest.Store.CheckDesync— the Store runsgit -C gitDiragainst the realfilesystem (no injectable seam), and
release-decide.shdoes these checks inlineanyway (separate from
manifest-check.sh§5), so an own-seam port keeps the wholedecision unit-testable.
json.Valid+ per-field extract,jq .field // emptysemantics) — tolerating unknown fields + an absent
last_released_at, exactly asthe oracle does.
manifest.Store.Readis C2-stricter (DisallowUnknownFields) andwould reject manifests bash accepts. This is the ratified C2-lenient call from
#558, applied by precedent.
PREP_SUBJECT_RE(the Layer-1 trigger) is ported here — decide is its first Gomatch-consumer (
internal/preponly writes the literalchore(release): prepare <tag>, it never matches one).Verification
internal/decide): every mode (noop×2, update feat/fix/fragment/override, cut head/squash/buried), bootstrap (stable-tag + #368 virtual), the
#380/#56/#417/schema FATALs, the Layer-2/3 fall-through (fake forge), and the
conflicting-bump-label FATAL.
forgejo:ListPRs+ Author/Labels unmarshal.cmd/rt/decide_equiv_test.go): a prebuiltrtbinary vsrelease-decide.sh, byte-compared stdout + exit over noop / update / cut /#368 / #380 on a shared SHA-deterministic git-bootstrap wrapper (fixed identity
head_sha/last_released_shamatch between sides — thebytes a code read can't verify.
Layer-2 safeguard (
return "pass") reds the fall-through test (mode=cut layer2= passinstead ofupdate + safeguard_fail=layer2); a constanthead_shareds theequivalence cut case (proves the harness byte-compares the real SHA — non-vacuous).
forgejo-ci-go:latest(the harness execs bash +git in-container).
Coverage note (naming the surface, not just "harness confirms") — F5
The equivalence harness covers the git + config + dry-run modes (the SHA-bearing
stdout + the mode key-order). It does not drive the live-API paths — the
cut-safeguard Layer-2/3 FAIL cases and the rolling-PR bump-label read need canned API
responses injected on BOTH sides (bash's
FORGEJO_TEST_PR_LOOKUP_FILE/FORGEJO_TEST_OPEN_PRS_FILEvs a Go fake forge). Wiring those seams through theharness is high-cost for the marginal byte; they are covered by
internal/decide'sunit tests (fake forge) + the bash bats, and the reads themselves
(
FindPRByMergeSHA/ListPRs) rideinternal/forgejo's own equivalence. Samestdout-vacuous-for-API-paths split #559 established.
Bounded disclosed divergence (outside the compared surface): when
release-toolkit.ymlis absent, bash's
config_render_tagyields an empty tag while the Go portfalls back to
v<version>. Production + the bats oracle always provide a config (theharness wrapper does too), so the rendered tag matches everywhere the oracle exercises.
What this PR does NOT do
reusable-release.ymlstill calls the bashrelease-decide.sh; switching the call site is a later integration step (as#556–#559 left their orchestrators unwired).
released bash tool.
decidehelp short unchanged — the skeleton short is reused, socmd/rt/testdata/help.txtneeds no regeneration.Merge-order note
Branched off current
main(c1cf08a, post-#558/#559). The onlycmd/rt/main.gooverlap is the
implementedCmdsmap (adds thedecidekey) — independent of thesibling orchestrator keys.
Refs #555
Review — PR#566, rt decide (Go port of release-decide.sh, #555)
Independent deep-verify at head
7f70d35(basemain@c1cf08a). +2092/-6, 13 files. Ran the full gate, read the 826-LOC oracle against the 707-LOC port line-for-line, and built my own differential on the F5 out-of-harness surface (a real bash-vs-rtrun, not the shipped harness). That differential found one real divergence on the graded surface.One must-fix. One should-consider. Everything else — including all six flagged design calls — verified faithful. Details below.
Verified faithful (no action)
go build/vet/golangci-lint(0 issues)/go test ./... -count=1green (go1.26.2), incl.internal/decide+ theTestDecideEquivalenceharness. CI 6/8 green (shellcheck+bats still pending at review time — I'll want them green before any stamp).emit()order in the oracle. The 4event_emit cut_decidedcalls are stderr-only → invisible on the compared stdout (design call #2 faithful, same retire-with-reason precedent as the other 4 orchestrators).loadManifest/jqFieldmirrorsjq -r '.field // empty'(tolerates unknown fields + absentlast_released_at), NOTStore.Read's C2-strict — design call #3, the #558-ratified precedent.GitRunner, notmanifest.Store.CheckDesync(Store's non-injectablegit -C gitDir) — keeps the decision unit-testable; bash does these inline too. Design call #4 faithful.IndexByte(':')+ slice == bash${head_label#*:}, incl. the no-colon (unchanged) + empty-label edges (#92 head.label-not-head.ref).FindPRByMergeSHA404→ErrNotFound→err!=nil→"fail"== bash empty-pr_json→fail.ListPRsempty→empty-slice/nil-err→BumpNone== bash no-PR→return 0. Substrate additions (PullRequest.User/.Labels,ListPRs) carry correct json tags + strict-only-on-.number.PREP_SUBJECT_RE; group-2 version ==BASH_REMATCH[2]; bothprepare-prefix + squash-title styles (#331).v<version>fallback is the safer of the two (bash emits an emptytag=). Accepted as disclosed.MUST-FIX — #417 orphan check fires too narrowly (undisclosed graded-surface divergence)
The port made a dead bash variable load-bearing, and it changes the exit code.
In
release-decide.shthe orphan FATAL fires oncmp==1 && empty(_skip_reason)(lines 366–368)._orphan_trigger(assigned :361) is never read in the fire condition — I confirmed it's dead (grep '_orphan_trigger'→ only two assignments, zero reads). So bash halts wheneverCHANGELOG top-version > manifest versionand the walk finds no prep-PR-merge for the top version — regardless of whether a release-relevant commit exists.checkOrphanChangelog(decide.go:283–295) fires the orphan error only whenreleaseRelevant()finds a bump-worthy commit; its post-loopreturn nillets the "cmp==1but no release-relevant commit" case proceed instead of halting.Empirically demonstrated — real
release-decide.sh --dry-runvs thertbinary, byte-compared:0.2.0, manifest0.1.0, HEAD==manifest sha (0 commits)mode=noopdocs:commit on top (no prep-merge)mode=noop(no_release_relevant_content)feat:commit on top (genuine orphan)Impact. In a broken-state repo where the CHANGELOG documents a version the manifest hasn't caught up to and there's no bump-worthy commit yet, bash halts to force reconciliation; the port silently emits
mode=noopand the workflow proceeds as if healthy — precisely the silent-desync #417 exists to prevent. It is on the graded surface (exit code + stdout), it is not disclosed as a bounded divergence, and it sits inside the harness's own stated scope ("modes that depend only on git + config") yet none of the 5 shipped fixtures exercises the orphan path at all.Fix direction. After the walk, when
cmp==1and no skip was found, fire the orphan FATAL — matching bash's unconditionalelif. Keep the release-relevant→break(bash breaks on the trigger before reaching an older prep-merge-for-topversion, so a release-relevant commit newer than an old prep-merge must still halt). Net: the loop's only job is to detect the skip (a prep-merge-for-topversion seen before any release-relevant commit); every other loop exit → FATAL.Regression pin. Add an orphan-zero-trigger case to
decide-oracle.sh+TestDecideEquivalence— a fixture, not a self-authored expected string (the #558 lesson): CHANGELOG top ahead + a non-bump commit, assert exit 1 both sides. That closes the harness gap that let this through.SHOULD-CONSIDER — #86 Layer-2 post-merge-race retry not ported (disclosed, off-graded-surface)
You flagged the Layer-2 retry mapping and asked me to confirm it's faithful. It isn't, for the case #86 was built for.
check_layer2_branch_sourceretries the merge-sha lookup up to1 + 3×5s = 15swhile it returns empty — the Forgejo commit→PR index lags ~8s post-merge (#86, empirically hit on the v0.5.0 cut).checkLayer2callsFindPRByMergeSHAonce and relies on the client's retry — butcallRetry(client.go:260–262) retries only 429 + 5xx/network; a 404 (no-PR-yet during indexing) returnsErrNotFoundimmediately → Layer-2fail→mode=updatefall-through (safeguard_fail=layer2). The client's resilience is a different failure class than the 404-index-lag #86 addresses.So on a genuine cut where the prep-PR merge just landed and the index is still lagging, bash self-heals within 15s →
mode=cut; the port fails fast → the cut is silently downgraded to update on that run. Off the graded surface (no live API in the harness) and self-healing (a re-trigger picks it up once indexed), so non-blocking on the differential — but it drops a protection added for an observed race. Options: port a retry-on-ErrNotFoundintocheckLayer2/3(where bash puts it — not the client, sinceGetReleaseByTag's idempotency pre-check wants a fast 404), or file a follow-up tracker documenting the accepted delta. Your call on whether v1.0.0 wants parity here.Verdict
REQUEST_CHANGES, head-pinned at
7f70d35, on the one must-fix (the #417 orphan divergence — exit-code + stdout, undisclosed, inside the harness's own scope). The should-consider is yours to weigh, not a blocker. Everything else is a clean, faithful, well-documented port and the six design calls all hold up. Re-request my stamp after the orphan fix + its harness fixture land and I'll re-verify byte-for-byte on the new differential.— Surveyor
Re-review — PR#566, rt decide (#555) — both 4621 findings closed
Re-verified at head
22507cf(clean fixup onmain@c1cf08a, no rebase). The delta touches exactly the 5 expected files (bothdecide.go, the oracle wrapper, the equiv test, the unit tests) — nothing else. I re-ran my own orphan differential on the rebuilt binary and mutation-verified the new regression pins independently, rather than reading the fix.MUST-FIX — #417 orphan check (4621) — FIXED + pinned
breaks (not return); the post-loopreturn nil→return orphanFATAL. So the loop's only non-FATAL exit is the prep-merge-for-topversion skip — exactly bash'scmp==1 && empty(_skip_reason)fire condition, with the release-relevant→break preserving bash's break-before-an-older-prep-merge semantics.rt,--dry-run, byte-compared)tag=boundary — confirmed against the oracle wrapper's own comment, not a regression.)orphan-no-triggerscenario indecide-oracle.sh+ a 6thTestDecideEquivalencecase (byte-diffs vs real bash), plus 3 unit tests (TestDecide_417OrphanFatal/…NoRelevantCommit/…EmptyWalk— named for cases (b) and (a)). A fixture, not a self-authored string — the #558 lesson.return nilreds the#417 orphanequiv case (bash exit 1 vs go exit 0mode=noop, on both exit_code + stdout) and all 3 unit tests. Reverted by discarding the copy (rt566b pristine, 0 mutation markers).SHOULD-CONSIDER — #86 Layer-2 retry — PORTED (parity)
You chose parity over defer — right call for the final orchestrator. Verified faithful:
lookupMergedPRretries only onErrNotFound(errors.Is); a non-404 error returns immediately — no double-retry against the client's own 429/5xx retry.Layer2RetryCount+1attempts,Layer2RetryDelaybetween (bashLAYER2_RETRY_COUNT=3/DELAY=5s; tests inject 0). Layer 3 does NOT retry — a single lookup, by which point Layer 2's loop has absorbed the index lag (bashcheck_layer3truth). ✅envInt("LAYER2_RETRY_COUNT",3)/("LAYER2_RETRY_DELAY",5)wiring matches bash's${VAR:-default}. ✅TestDecide_cutLayer2RetrySucceeds(empty×N→found→cut) +TestDecide_cutLayer2RetryExhausted(budget→fall-through) both pass, and both red when I disable the retry (attempts:=1) — non-vacuous. ✅Gate
✅
go build/vet/golangci-lint(0 issues)/go test ./... -count=1green (go1.26.2), incl. the 6-case equiv harness + all decide unit tests. CI combined-success 8/8 on22507cf.Verdict
APPROVED, head-pinned at
22507cf. Both 4621 findings are closed, byte-verified against real bash on my own differential and mutation-confirmed load-bearing, the orphan fix doesn't over-fire on any legit state, and the #86 parity port is faithful + tested. This is the last orchestrator — Phase 6's decide step lands clean. Yours to land.— Surveyor