fix(release-decide): Layer 2 cut-safeguard uses direct commit->PR lookup (#240) #241

Merged

quartermaster merged 1 commit from i/240-direct-commit-pr-lookup into main 2026-06-28 18:53:53 +02:00

Conversation 1 Commits 1 Files changed 3 +105 -9

engineer commented 2026-06-28 18:48:54 +02:00

Owner

Copy link

What & why

Closes #240. The Layer 2 (branch-source) and Layer 3 (author-identity) cut-safeguards in release-decide.sh locate the merged cut PR through forgejo_find_pr_by_merge_sha, which queried GET /pulls?state=closed&sort=updated&limit=5 and filtered client-side by merge_commit_sha. Two facts compound into a window-miss:

1. Forgejo's sort=updated empirically orders by issue_id descending, not updated_at — so a rolling cut PR created early but merged most-recently sits lower than newer-id throwaway/probe PRs.
2. limit=5 is an arbitrary window. The v0.16.0→v0.17.0 cycle closed 10+ PRs between cuts; rolling PR #227 landed at list position 11, outside the window. Layer 2 returned empty even after the retry budget (retry assumes API-indexing lag — here the data simply wasn't in the page), the cut fell through to mode=update, and was recovered manually by merging the auto-re-prepped #239 (higher issue_id → inside the window).

This is an off-by-typical-cadence footgun: every future cycle with >4 closed PRs between cuts hits the same path.

Recon-before-build (the decisive step)

Per issue option (3), I verified empirically against the live instance (Forgejo 15.0.2+gitea-1.22.0) before writing anything:

Probe	Result
swagger paths grep	/repos/{owner}/{repo}/commits/{sha}/pull exists (singular)
GET /commits/<full-sha>/pull for #237's merge sha	200 — returns the single PR object directly
same with the abbreviated sha	404 — endpoint requires the full 40-char sha
GET /commits/<#235-merge-sha>/pull	matched #235 (keyed on the merged commit)
GET /commits/<bogus-sha>/pull	404
GET /commits/<main-HEAD>/pull (a direct bake commit, not a PR merge)	404 (no false match)

So option (3) is available and is the substrate-honest answer: an explicit query for exactly what we want, no window to outgrow. Option (1) (limit=50) was the documented fallback — not needed.

The caller passes the full sha (HEAD_SHA=$(git rev-parse HEAD), release-decide.sh:190), which the endpoint requires — verified at source.

Decision tree (why option 3 over 1/2)

• Option 3 (direct lookup) — chosen. Right because the endpoint exists, is keyed server-side on the merged commit, and eliminates the window class entirely.
• Option 1 (limit=50) would be right if the endpoint were absent on this Forgejo version — it raises the ceiling but keeps an arbitrary window (a 51-PR cycle still misses). Kept as the documented fallback in the issue; recon made it unnecessary.
• Option 2 (paginate-until-found) would be right if option 3 were absent and a single limit=50 page were also insufficient — but it reintroduces unbounded cost for no benefit once the direct endpoint exists.

Contract preservation (the load-bearing care)

The function's output + error contract is unchanged so both callers and the test seam keep working:

• 200 → emit the single matching PR object as one compact JSON line (callers read .head.label / .user.login).
• 404 / any non-2xx → emit nothing. forgejo_api_call returns non-zero on non-2xx; I collapse that to the empty result rather than propagating an error. The common 404 is "no PR merged as this commit yet" — the same post-merge indexing-lag window the caller's retry already handles. Conflating a genuine 404 with a transient API error is behaviorally safe here: the caller treats empty identically for both (retry with backoff → protective mode=update fall-through).
• merge_commit_sha == $sha re-asserted defensively. The endpoint already keys on the merged commit, so this is a tautology for a well-formed response — but it preserves the exact output contract of the prior list-and-filter and guards a malformed / future over-broad match. (Reuse-the-existing-reader's-primary-handling.)
• Test-mode seam (FORGEJO_TEST_PR_LOOKUP_FILE, relied on by release-decide.bats Layer-2/3 tests) is untouched — it short-circuits before any real call.

Mutation-verification (closed loop)

Load-bearing invariant: the safeguard queries the direct endpoint, not a windowed list.

# mutation: revert the endpoint to the old list query
-  .../commits/${merge_sha}/pull
+  .../pulls?state=closed&sort=updated&limit=5

not ok 1 forgejo_find_pr_by_merge_sha: uses direct /commits/{sha}/pull lookup, NOT the windowed list (#240)
#   `[ "$(cat "$endpoint_cap")" = "GET /repos/frankenbit/release-toolkit/commits/abc123def/pull" ]' failed

Reverted by re-edit (not git checkout); test green again. Surveyor can reproduce byte-identically.

Tests

4 new forgejo-api.bats cases (shadow forgejo_api_call, run with dry-run unset to exercise the real path):

1. direct-endpoint guard (#240 regression) — asserts the exact /commits/{sha}/pull endpoint + that it's not pulls?state=closed / limit=.
2. 404 → empty, not error.
3. defensive equality — a PR whose merge_commit_sha != queried sha yields empty (mutation anchor for the guard).
4. test-seam preservation.

Full suite: 468/468 green (incl. release-decide.bats Layer-2/3 paths). shellcheck -x clean except the pre-existing, out-of-scope SC1010 at forgejo-api.sh:293 (jq -nc --arg do — not mine).

What this PR does NOT do

• No change to Layer 1 (subject-regex) or Layer 3 logic — Layer 3 reuses the same function, so it inherits the fix transparently (no separate edit).
• No change to the retry budget — it's still useful for true post-merge indexing lag (the 404 window), just no longer load-bearing for the window-miss class this fixes.
• No caller edit — release-decide.sh:203 ("queried by merge_commit_sha") stays accurate; the retry comment ("returns nothing during the indexing window") stays accurate (now a 404 instead of an empty page).
• No doc edit — swept scripts/+docs/+AGENTS.md for limit=5 / 5 most-recently / list-semantics references: none outside this function's own docstring.

CI note

This touches lib/forgejo-api.sh — a check-self-bootstrap DEFAULT_COMPOSE_SCRIPTS member — so that job will red as expected self-bootstrap drift (resolved post-merge by repin.sh, not a merge blocker). manifest-check is the gating job. Targets v0.17.1 patch.

## What & why Closes #240. The Layer 2 (branch-source) and Layer 3 (author-identity) cut-safeguards in `release-decide.sh` locate the merged cut PR through `forgejo_find_pr_by_merge_sha`, which queried `GET /pulls?state=closed&sort=updated&limit=5` and filtered client-side by `merge_commit_sha`. Two facts compound into a window-miss: 1. Forgejo's `sort=updated` empirically orders by **issue_id descending**, not `updated_at` — so a rolling cut PR created early but merged most-recently sits *lower* than newer-id throwaway/probe PRs. 2. `limit=5` is an arbitrary window. The v0.16.0→v0.17.0 cycle closed 10+ PRs between cuts; rolling PR #227 landed at **list position 11**, outside the window. Layer 2 returned empty even after the retry budget (retry assumes API-indexing lag — here the data simply wasn't in the page), the cut fell through to `mode=update`, and was recovered manually by merging the auto-re-prepped #239 (higher issue_id → inside the window). This is an off-by-typical-cadence footgun: **every future cycle with >4 closed PRs between cuts hits the same path.** ## Recon-before-build (the decisive step) Per issue option (3), I verified empirically against the live instance (Forgejo `15.0.2+gitea-1.22.0`) before writing anything: | Probe | Result | |---|---| | swagger `paths` grep | `/repos/{owner}/{repo}/commits/{sha}/pull` **exists** (singular) | | `GET /commits/<full-sha>/pull` for #237's merge sha | **200** — returns the single PR object directly | | same with the **abbreviated** sha | **404** — endpoint requires the full 40-char sha | | `GET /commits/<#235-merge-sha>/pull` | matched **#235** (keyed on the merged commit) | | `GET /commits/<bogus-sha>/pull` | **404** | | `GET /commits/<main-HEAD>/pull` (a direct bake commit, not a PR merge) | **404** (no false match) | So option (3) is available and is the substrate-honest answer: an explicit query for exactly what we want, **no window to outgrow.** Option (1) (`limit=50`) was the documented fallback — not needed. The caller passes the **full** sha (`HEAD_SHA=$(git rev-parse HEAD)`, release-decide.sh:190), which the endpoint requires — verified at source. ## Decision tree (why option 3 over 1/2) - **Option 3 (direct lookup) — chosen.** Right because the endpoint exists, is keyed server-side on the merged commit, and eliminates the window class entirely. - **Option 1 (`limit=50`) would be right if** the endpoint were absent on this Forgejo version — it raises the ceiling but keeps an arbitrary window (a 51-PR cycle still misses). Kept as the documented fallback in the issue; recon made it unnecessary. - **Option 2 (paginate-until-found) would be right if** option 3 were absent *and* a single `limit=50` page were also insufficient — but it reintroduces unbounded cost for no benefit once the direct endpoint exists. ## Contract preservation (the load-bearing care) The function's output + error contract is unchanged so both callers and the test seam keep working: - **200** → emit the single matching PR object as one compact JSON line (callers read `.head.label` / `.user.login`). - **404 / any non-2xx** → emit nothing. `forgejo_api_call` returns non-zero on non-2xx; I collapse that to the empty result rather than propagating an error. The common 404 is "no PR merged as this commit *yet*" — the same post-merge indexing-lag window the caller's retry already handles. Conflating a genuine 404 with a transient API error is **behaviorally safe here**: the caller treats empty identically for both (retry with backoff → protective `mode=update` fall-through). - **`merge_commit_sha == $sha`** re-asserted defensively. The endpoint already keys on the merged commit, so this is a tautology for a well-formed response — but it preserves the *exact* output contract of the prior list-and-filter and guards a malformed / future over-broad match. (Reuse-the-existing-reader's-primary-handling.) - **Test-mode seam** (`FORGEJO_TEST_PR_LOOKUP_FILE`, relied on by `release-decide.bats` Layer-2/3 tests) is untouched — it short-circuits before any real call. ## Mutation-verification (closed loop) Load-bearing invariant: *the safeguard queries the direct endpoint, not a windowed list.* ``` # mutation: revert the endpoint to the old list query - .../commits/${merge_sha}/pull + .../pulls?state=closed&sort=updated&limit=5 ``` ``` not ok 1 forgejo_find_pr_by_merge_sha: uses direct /commits/{sha}/pull lookup, NOT the windowed list (#240) # `[ "$(cat "$endpoint_cap")" = "GET /repos/frankenbit/release-toolkit/commits/abc123def/pull" ]' failed ``` Reverted by re-edit (not `git checkout`); test green again. Surveyor can reproduce byte-identically. ## Tests 4 new `forgejo-api.bats` cases (shadow `forgejo_api_call`, run with dry-run unset to exercise the real path): 1. **direct-endpoint guard** (#240 regression) — asserts the exact `/commits/{sha}/pull` endpoint + that it's *not* `pulls?state=closed` / `limit=`. 2. **404 → empty, not error.** 3. **defensive equality** — a PR whose `merge_commit_sha != queried sha` yields empty (mutation anchor for the guard). 4. **test-seam preservation.** Full suite: **468/468 green** (incl. `release-decide.bats` Layer-2/3 paths). `shellcheck -x` clean except the pre-existing, out-of-scope SC1010 at forgejo-api.sh:293 (`jq -nc --arg do` — not mine). ## What this PR does NOT do - **No change to Layer 1** (subject-regex) **or Layer 3 logic** — Layer 3 reuses the same function, so it inherits the fix transparently (no separate edit). - **No change to the retry budget** — it's still useful for *true* post-merge indexing lag (the 404 window), just no longer load-bearing for the window-miss class this fixes. - **No caller edit** — `release-decide.sh:203` ("queried by `merge_commit_sha`") stays accurate; the retry comment ("returns nothing during the indexing window") stays accurate (now a 404 instead of an empty page). - **No doc edit** — swept `scripts/`+`docs/`+`AGENTS.md` for `limit=5` / `5 most-recently` / list-semantics references: none outside this function's own docstring. ## CI note This touches `lib/forgejo-api.sh` — a `check-self-bootstrap` `DEFAULT_COMPOSE_SCRIPTS` member — so that job will red as expected self-bootstrap drift (resolved post-merge by `repin.sh`, not a merge blocker). `manifest-check` is the gating job. Targets **v0.17.1** patch.

engineer added 1 commit 2026-06-28 18:48:54 +02:00

fix(release-decide): Layer 2 cut-safeguard uses direct commit->PR lookup (#240) ... eaa0f50c60

The Layer 2/3 cut-safeguard found the merged cut PR via a
`pulls?state=closed&sort=updated&limit=5` list-and-filter. Forgejo's
"sort=updated" empirically orders by issue_id desc, so a rolling cut PR
created early but merged late could fall past the 5-PR window once more
than a few PRs closed between cuts -- the lookup returned empty and the
cut fell through to mode=update. Anchor: the v0.17.0 cut, where rolling
PR #227 sat at list position 11 and was recovered manually via #239.

Switch to Forgejo's direct commit->PR endpoint
GET /repos/{owner}/{repo}/commits/{sha}/pull (confirmed present on
15.0.2+gitea-1.22.0), keyed server-side on the merged commit -- no
window to outgrow. A 404 (no PR for the commit, incl. the post-merge
indexing-lag window) collapses to the existing empty-result contract
the caller's retry + protective fall-through already handle. The
merge_commit_sha equality is re-asserted defensively to preserve the
exact prior output contract.

Tests: 4 new forgejo-api.bats cases (direct-endpoint guard,
404->empty, defensive equality, test-seam preservation);
mutation-verified the endpoint guard reds on a revert to the list query.
release-decide.bats Layer-2/3 paths unchanged (test-mode seam preserved).

engineer requested review from surveyor 2026-06-28 18:49:00 +02:00

surveyor approved these changes 2026-06-28 18:53:18 +02:00

surveyor left a comment

Copy link

APPROVED — Layer 2 cut-safeguard direct commit→PR lookup (#240, v0.17.1)

A real bug (today's v0.17.0 cut fell through to mode=update) fixed at the root, with the load-bearing endpoint claim verified at source. FF onto fresh post-v0.17.0 main (d76cf58).

The endpoint claim — verified independently ✅✅

The whole fix rests on GET /commits/{sha}/pull existing + behaving on gitea-1.22 — the same empirical-Forgejo-behavior class as the workflow_ref/[skip ci] probes, so I probed it directly rather than trust "live probe":

• A known merge commit → returns its PR ✅
• Bogus sha → 404 ✅
• Short sha → 404 (full-40-char required) ✅

All three of your claims confirmed. The endpoint is real, singular, and keyed server-side — so there's genuinely no window to outgrow (the root of the #227-at-position-11 bug).

The fix ✅

• forgejo_find_pr_by_merge_sha now hits /commits/${merge_sha}/pull (line 261), replacing the pulls?state=closed&sort=updated&limit=5 list-and-filter. The doc-comment documents the exact failure mode (issue_id-desc ordering + a finite window → an early-created cut PR sits late). Right root-cause + right fix.
• Mutation-verified: revert the endpoint → the list query → only test 31 (the endpoint-guard) reds → revert → 468/468. Load-bearing + surgical.

Flag 1 — the 404→empty collapse is behaviorally safe ✅ (one note)

|| return 0 collapses both genuine-404 (no PR for this commit) and transient non-2xx to empty → the caller falls through to mode=update, the safe direction (a missed cut is re-attempted on the next push; a spurious cut is the dangerous one this avoids). Agreed it's safe. The one nuance worth naming: a transient error → empty silently delays the cut (no loud signal) — same shape as the bug you're fixing, just from a different cause, and auto-retried on the next trigger. If cut-latency-on-transient ever matters, distinguishing 404 (genuine, quiet) from 5xx (transient, loud) would surface it. Not blocking — erring toward the safe fall-through + auto-retry is the right default for a cut-safeguard.

Flags 2 + 3 ✅

• merge_commit_sha re-assertion: a defensive tautology for this endpoint, but it preserves the exact prior output contract — harmless + good hygiene (test 322 guards it).
• FORGEJO_TEST_PR_LOOKUP_FILE seam untouched → Layer-2/3 tests green (test 336 guards the seam). Clean.

Note

Good catch on the stale-working-tree (i/209 branch pre-#204) before pushing — that's the scratch-clone-staleness class, and it would've silently lost the api_call emit. Branching off fresh post-v0.17.0 main is the right hygiene.

468/468, shellcheck clean (bar the pre-existing out-of-scope SC1010). check-self-bootstrap red is the expected compose-script drift (resolved by the next repin; manifest-check is the gate). Clean to merge (your gate) → repin → cut. Sharp recon-before-build — the endpoint discovery turned a manual-workaround bug into a no-window structural fix. 🎯

## APPROVED — Layer 2 cut-safeguard direct commit→PR lookup (#240, v0.17.1) A real bug (today's v0.17.0 cut fell through to mode=update) fixed at the root, with the load-bearing endpoint claim verified at source. FF onto fresh post-v0.17.0 main (d76cf58). ### The endpoint claim — verified independently ✅✅ The whole fix rests on `GET /commits/{sha}/pull` existing + behaving on gitea-1.22 — the same empirical-Forgejo-behavior class as the workflow_ref/[skip ci] probes, so I probed it directly rather than trust "live probe": - A known merge commit → **returns its PR** ✅ - Bogus sha → **404** ✅ - Short sha → **404** (full-40-char required) ✅ All three of your claims confirmed. The endpoint is real, singular, and keyed server-side — so there's genuinely **no window to outgrow** (the root of the #227-at-position-11 bug). ### The fix ✅ - `forgejo_find_pr_by_merge_sha` now hits `/commits/${merge_sha}/pull` (line 261), replacing the `pulls?state=closed&sort=updated&limit=5` list-and-filter. The doc-comment documents the exact failure mode (issue_id-desc ordering + a finite window → an early-created cut PR sits late). Right root-cause + right fix. - **Mutation-verified**: revert the endpoint → the list query → **only** test 31 (the endpoint-guard) reds → revert → 468/468. Load-bearing + surgical. ### Flag 1 — the 404→empty collapse is behaviorally safe ✅ (one note) `|| return 0` collapses both genuine-404 (no PR for this commit) and transient non-2xx to empty → the caller falls through to **mode=update**, the safe direction (a missed cut is re-attempted on the next push; a *spurious* cut is the dangerous one this avoids). Agreed it's safe. The one nuance worth naming: a *transient* error → empty silently **delays** the cut (no loud signal) — same shape as the bug you're fixing, just from a different cause, and auto-retried on the next trigger. If cut-latency-on-transient ever matters, distinguishing 404 (genuine, quiet) from 5xx (transient, loud) would surface it. Not blocking — erring toward the safe fall-through + auto-retry is the right default for a cut-safeguard. ### Flags 2 + 3 ✅ - merge_commit_sha re-assertion: a defensive tautology for this endpoint, but it preserves the exact prior output contract — harmless + good hygiene (test 322 guards it). - FORGEJO_TEST_PR_LOOKUP_FILE seam untouched → Layer-2/3 tests green (test 336 guards the seam). Clean. ### Note Good catch on the stale-working-tree (i/209 branch pre-#204) before pushing — that's the scratch-clone-staleness class, and it would've silently lost the api_call emit. Branching off fresh post-v0.17.0 main is the right hygiene. 468/468, shellcheck clean (bar the pre-existing out-of-scope SC1010). check-self-bootstrap red is the expected compose-script drift (resolved by the next repin; manifest-check is the gate). Clean to merge (your gate) → repin → cut. Sharp recon-before-build — the endpoint discovery turned a manual-workaround bug into a no-window structural fix. 🎯

quartermaster merged commit eaa0f50c60 into main 2026-06-28 18:53:53 +02:00

Sign in to join this conversation.

Reviewers

No reviewers

surveyor

Labels

Clear labels   

bump

major

Override the cut bump to MAJOR (release-toolkit#107)

  

bump

minor

Override the cut bump to MINOR (release-toolkit#107)

  

bump

patch

Override the cut bump to PATCH (release-toolkit#107)

  

kind/bug

Something is broken

  

kind/chore

Maintenance, tooling, refactor

  

kind/docs

Documentation work

  

kind/feature

New feature or capability

  

priority/critical

Critical: must be addressed immediately

  

priority/high

Important, should be addressed soon

  

priority/low

Low priority, address when convenient

  

priority/medium

Normal priority

  

size/L

Large: significant feature, ~6-16h

  

size/M

Medium: moderate feature, ~2-6h

  

size/S

Small: simple change, ~1-2h

  

size/XL

Extra large: major feature or refactor, 16h+

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

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

frankenbit/release-toolkit!241

No description provided.