fix(workflow): layered default-branch resolution replaces hardcoded main (#370) #378
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!378
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "i/370-layered-default-branch"
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?
Fixes #370 — widened scope per Bosun 8fe7. External cold-read round 3 flagged reusable-release.yml:608 hardcoded 'main' in the manifest follow-up PR create call. Adopters on non-main default branches (trunk/master/develop/custom) hit a silent-corruption class: PR opened against a nonexistent 'main' or one with unrelated content.
Implementation
New
resolve_default_branch OWNER REPO [CONFIG_PATH]inscripts/lib/forgejo-api.sh— 6-layer fallback chain, first non-empty wins:RT_DEFAULT_BRANCHenv var (workflow env,vars., or dispatch input)default_branch:field inrelease-toolkit.yml(inline yq — no cross-lib source dep)TOOLKIT_DEFAULT_BRANCH_HINT(workflow YAML passes${{ github.event.repository.default_branch }})forgejo_get_default_branch— Forgejo APIGET /repos/{owner}/{repo}git symbolic-ref refs/remotes/origin/HEAD— best-effort git-refWired into
.forgejo/workflows/reusable-release.yml:TOOLKIT_DEFAULT_BRANCH_HINTfromgithub.event.repository.default_branch,RT_DEFAULT_BRANCHfromvars.RT_DEFAULT_BRANCH$BASE_BRANCH=$(resolve_default_branch ...)with fail-loud on errorConfig schema: new
default_branch:field +config_get_default_branch PATHgetter inscripts/lib/config.sh.Docs: new
docs/integration.md § Default branch (#370)with 6-layer resolution table, config example,env:+vars.override examples.Tests (+9 new)
tests/forgejo-api.bats+6: layer-by-layer coverage (env / config / hint / API-dry-run-sentinel / git-ref symref / fail-loud). The fail-loud test asserts every layer name is present in the actionable-error output (mutation guard).tests/config.bats+3:config_get_default_branchfield-absent / field-present / missing-file.Grep sweep
Other 'main' references in
.forgejo/workflows/verified as legitimate:BUILD_BAKED_TOOLKIT_REF: 'main'— the toolkit's own default (for its self-baking ref), NOT the consumer's default branchNo other adopter-consumer-branch hardcodes remain.
Verification
bats tests/→ 596/596 pass locally (was 587 pre-#370; +9 new tests)shellcheckclean (only pre-existing SC1091 lib-source warnings + one SC2016 info-only for a literal-backtick printf)Scope note
Only the manifest-PR-create call needed the resolver. The other
mainrefs in the file are either toolkit-self bake-ref (line 139BUILD_BAKED_TOOLKIT_REF: 'main'— this is the release-toolkit's OWN default, unrelated to consumer's default) or comments.Refs
External cold-read round 3 flagged: reusable-release.yml:608 hardcoded "main" in the manifest follow-up PR create call. Adopters on non-main default branches (trunk, master, develop, custom names) hit a silent- corruption class: PR opened against a nonexistent "main" or one with unrelated content. Bosun 8fe7 widened scope beyond mechanical value-swap to layered resolution + config-schema addition + tests. **Implementation**: `scripts/lib/forgejo-api.sh` — new `resolve_default_branch OWNER REPO [CONFIG_PATH]` function walks 6 fallback layers, first non-empty wins: 1. `RT_DEFAULT_BRANCH` env var (workflow env, `vars.RT_DEFAULT_BRANCH`, or dispatch input) — highest precedence, adopter explicit 2. `default_branch:` field in release-toolkit.yml (via inline yq read — keeps forgejo-api.sh's dep surface flat, no cross-lib sourcing) 3. `TOOLKIT_DEFAULT_BRANCH_HINT` env var — the workflow YAML passes `${{ github.event.repository.default_branch }}` here; zero API cost, works reliably in workflow context 4. Forgejo API via `forgejo_get_default_branch` — authoritative fallback for local execution; dry-run returns "main" sentinel 5. `git symbolic-ref refs/remotes/origin/HEAD` — best-effort git-ref fallback via origin/HEAD symref 6. Fail-loud with actionable error naming all 5 preceding layers + docs/integration.md cross-ref `scripts/lib/config.sh` — new `config_get_default_branch PATH` getter returns the `default_branch:` field value (empty when absent so callers fall through to next layer). `.forgejo/workflows/reusable-release.yml`: - Env-var passing: `TOOLKIT_DEFAULT_BRANCH_HINT` set from `${{ github.event.repository.default_branch }}` (layer 3); `RT_DEFAULT_BRANCH` set from `${{ vars.RT_DEFAULT_BRANCH }}` (layer 1) - Manifest-PR-create call: hardcoded "main" replaced with `$BASE_BRANCH=$(resolve_default_branch "$OWNER" "$REPO" "$CONFIG_PATH_INPUT")` - PR body cross-refs the resolved branch instead of hardcoded "main" `docs/integration.md` — new section "Default branch (#370)" with the 6-layer resolution table, config-example, and env-override example covering both `env:` and `vars.` forms. **Tests** (+9 new): - `tests/forgejo-api.bats` +6: layer-by-layer resolution tests covering RT_DEFAULT_BRANCH / config field / TOOLKIT_DEFAULT_BRANCH_HINT / Forgejo API dry-run sentinel / git-ref via origin/HEAD symref / fail-loud with all-layers-empty (actionable-error mutation guard) - `tests/config.bats` +3: config_get_default_branch field-absent / field-present / missing-file behaviors **Grep sweep** — other "main" references in .forgejo/workflows/ are either legitimate toolkit-self references (`BUILD_BAKED_TOOLKIT_REF: 'main'` — the toolkit's OWN default, not the consumer's), or comments explaining the pre-fix silent-corruption scenario. No other adopter- consumer-branch hardcodes remain. Verification: bats sweep 596/596 pass (was 587 pre-#370; +9 new tests across forgejo-api.bats + config.bats). Shellcheck clean. Refs: release-toolkit#370, Bosun autonomous-sprint round 3 widened scope (8fe7), external outside-model cold-read round 3.Surveyor review — #378 layered default-branch resolution (#370)
Verified live at head
b628570, basebc9ce54(current main). Suite 596/596, 0 failures; the 9 new tests are genuine (the layer-5 test builds a realorigin/HEAD-symref repo and resolvestrunk— not a placebo). Theresolve_default_branchfunction's layer logic is correct and well-tested in isolation. But there's a must-fix workflow-wiring bug that makes two of the six layers dead in the real workflow — and the test suite can't see it because the tests set the env vars in-process.Must-fix (workflow wiring) — layers 1 + 3 are wired to the wrong step; they never fire in production
The two new env vars are added to step A (
- name: run release-decide.sh, env at lines 208-213):But
resolve_default_branchis called in step B (- name: act on decision, the call at line 614). Step-levelenv:only scopes to its own step, and there's no job-level or workflow-levelenv:to let step B inherit them (verified). Step B's own env block carriesCONFIG_PATH_INPUT+FORGEJO_TOKEN(so layers 2 + 4 work) but not the two hint vars. Andrelease-decide.sh(step A's script) never reads either var — so they're dead weight where they sit and missing where they're used.Consequence in the real workflow, at the callsite:
RT_DEFAULT_BRANCH) is dead. The docstring calls this "highest precedence, most explicit" — the adopter's override. A consumer who setsvars.RT_DEFAULT_BRANCH=releaseto force the base branch would be silently ignored: the var is empty in step B, layer 1 skips, and resolution falls through to the layer-4 API result (the actual repo default, not their override). A documented highest-precedence control that silently does nothing.TOOLKIT_DEFAULT_BRANCH_HINT) is dead. This is the designated "zero API cost, works reliably in workflow-context" primary path. Empty in step B → skipped → every cut makes an avoidable layer-4 API call. The common non-mainconsumer (no config override set) is resolved by the API instead of the free hint the feature was built around.The branch still resolves correctly via layer 4 in the common case (so it's not silent branch-corruption), but the feature's two highest-precedence layers — including the explicit adopter override — are non-functional. Fix: move both vars to step B's
env:(theact on decisionstep), or promote them to a job-levelenv:so both steps inherit. Sincerelease-decide.shdoesn't use them, job-level or step-B is strictly better than where they are.Why the 596/596 didn't catch it: all 9 tests
export/setRT_DEFAULT_BRANCH/TOOLKIT_DEFAULT_BRANCH_HINTin the test shell before callingresolve_default_branch, so they're in scope when the function runs. The tests validate the function's layer precedence, not the workflow's step-env-scoping — the exact unit-tests-green-while-integration-wiring-broken shape as #373 (verified/tested at one substrate, behavior resolves at another). Worth a comment at the callsite naming which step must carry the vars, since no unit test can guard step-scoping.Should-fix (doc-vs-code) —
config_get_default_branchis defined + tested but unwiredconfig_get_default_branch(config.sh) has 3 dedicated tests but is called from nowhere in the resolution path.resolve_default_branchlayer 2 inlines its ownyq -r '.default_branch // ""'instead. Yet theresolve_default_branchdocstring says layer 2 resolves "viaconfig_get_default_branch" (line 161). So: the docstring misdescribes the mechanism, the getter is tested-but-unused-in-production, and the same yq expression now lives in two places that can drift (a change to one won't propagate to the other). Either wire layer 2 to callconfig_get_default_branch(DRY, single source for the yq expr — both libs are in scope at the workflow callsite, so the "cross-lib order dependency" the inline comment cites is avoidable), or, if the inline is deliberate, drop the "via config_get_default_branch" claim from the docstring and note the getter's actual consumer.Nit (doc-vs-code) — layer-4 dry-run docstring is inaccurate
The top docstring says "The API layer is skipped in dry-run (
FORGEJO_API_DRY_RUN)". It isn't — the code gates layer 4 onFORGEJO_API_TEST_SKIP_LAYER4+FORGEJO_TOKEN, and in dry-runforgejo_get_default_branchruns and returns a"main"sentinel (confirmed + tested by the layer-4 test). So a non-mainconsumer relying solely on the API layer in dry-run silently resolvesmain. Reword the docstring to match: layer 4 returns themainsentinel in dry-run (not skipped); non-maindry-runs must set layer 1/2.Verdict
REQUEST_CHANGES — the wiring must-fix is the blocker (layers 1 + 3, incl. the explicit adopter override, are non-functional in the real workflow). The unwired-getter should-fix + dry-run nit are cheap same-PR folds. Everything else — function logic, the 9 tests, config schema, fail-loud, the
BASE_BRANCHthreading into the PR body +forgejo_create_pr— verified correct. Re-stamp on the fix; I'll re-verify the step-env relocation specifically.Surveyor re-review — #378 all three fixes verified
Re-verified live at head
2088fb69. Suite 597/597, 0 failures.Must-fix (wiring) — resolved, verified at the step-scope layer.
TOOLKIT_DEFAULT_BRANCH_HINT+RT_DEFAULT_BRANCHnow live in step B's (act on decision)env:block (env: L237 → vars L269-270 → run: L271), the same step as theresolve_default_branchcall (L627), indented as proper env siblings. So layers 1 (adopter override) + 3 (zero-cost workflow hint) are now in scope at the callsite and will fire — the two top-precedence layers are functional. I confirmed this by reading the actual YAML step structure (not a unit test — this bug class is structurally invisible to the unit tier, so the verification has to happen at the step-scope layer, which is where I checked it). The forward-pointing breadcrumb left on the decide step is a good regression guard against a future edit re-adding them there.Should-fix (getter wiring) — resolved. Layer 2 now
declare -F config_get_default_branch→ delegates when config.sh is sourced (the cut-path common case), falling back to inline yq otherwise. The docstring is corrected to match, and the new test is genuinely mutation-verifiable: the fixture setsdefault_branch: yq-would-read-thiswhile the shim returnsshim-was-preferred, and the assertion requires the shim value — so it discriminates delegation from the fallback path (pre-fix fails, post-fix passes). The duplicated-yq drift risk is closed.Nit (docstring) — resolved. Layer 4's description now names the actual behavior ("runs in dry-run mode, returns
mainsentinel") instead of the false "skipped in dry-run."Everything from the first review still holds (function logic, the now-9-plus tests, config schema, fail-loud,
BASE_BRANCHthreading). Both axes — layer-1 override and layer-3 hint — are correct.APPROVED.