frankenbit/release-toolkit

Fork 0

test(ci): two of lookup_pr's three refusals are unpinned #1301

New issue

Closed

opened 2026-09-06 12:13:47 +02:00 by bosun · 2 comments

bosun commented

2026-09-06 12:13:47 +02:00

Owner

lookup_pr promises three refusals and only one of them is pinned; the two unpinned ones fail in the direction the script exists to prevent.

Found by @quartermaster reviewing #1294, which implements #1278. Filed by @bosun.

Measured

non-2xx / 404 status            mutated to silently return {}   ✅ control 3 catches it
curl TRANSPORT failure          mutated to silently return {}   ❌ NOTHING REDDENS
200 with a non-{number:…} body  shape check removed             ❌ NOTHING REDDENS

⚠️ Its own doc comment promises all three: "any other API failure is a refusal: silently paging the tracker would hide a broken lookup."

🔑 Both gaps fail toward the same wrong answer: a broken lookup becomes "this commit has no PR", which pages the tracker and reads as a NORMAL FALLBACK. There is no red anywhere; the page just names the wrong owner.

📌 A transport failure is the likelier of the two in CI — a runner losing the API mid-run is ordinary, a malformed 200 is not.

✅ The behaviour is correct today. This is arm coverage, not a defect — two more table rows close it.

AC

An arm covers a curl TRANSPORT failure and asserts the refusal. tests/post-merge-build-page.bats:107 — "pager refuses a transport failure instead of paging a false no-PR result". Landed in #1359 (5398675).
An arm covers a 200 whose body is not {number: …}. :118 — "pager refuses a malformed 200 instead of paging a false no-PR result".
Each reddens on its own mutation. Verified against main AFTER the merge, not against the PR:\n\n\nM0 control rc=0\nM1 transport failure -> silently return {} rc=1\nM2 drop the 200 shape check rc=1\nrestored rc=0\n\n\nThe same two mutations stayed GREEN after #1352 merged, which is what reopened this.

#1278 / #1294 (the gate), #1270 (the composition failure control 4 reproduces)

Anchor

@quartermaster, 2026-09-06, reviewing a PR that implements a tracker he requested — disclosed on the review row, and the findings come from exercising the script rather than from reading it.

📌 He also checked the TRIGGER first, which is the step that makes the rest meaningful: go-ci carries both pull_request and push on [main, v2/next], and there is a live go-ci / lint + build + test (push) success on main's own head, so the new job will actually fire. This repo produced the opposite shape twice today — #1271's fourteen (push) contexts that no (pull_request) required set can classify.

`lookup_pr` promises three refusals and only one of them is pinned; the two unpinned ones fail in the direction the script exists to prevent. Found by @quartermaster reviewing `#1294`, which implements `#1278`. Filed by @bosun. ## Measured ``` non-2xx / 404 status mutated to silently return {} ✅ control 3 catches it curl TRANSPORT failure mutated to silently return {} ❌ NOTHING REDDENS 200 with a non-{number:…} body shape check removed ❌ NOTHING REDDENS ``` ⚠️ **Its own doc comment promises all three:** *"any other API failure is a refusal: silently paging the tracker would hide a broken lookup."* 🔑 **Both gaps fail toward the same wrong answer: a broken lookup becomes *"this commit has no PR"*, which pages the tracker and reads as a NORMAL FALLBACK.** *There is no red anywhere; the page just names the wrong owner.* 📌 **A transport failure is the likelier of the two in CI** — a runner losing the API mid-run is ordinary, a malformed 200 is not. ✅ **The behaviour is correct today. This is arm coverage, not a defect** — two more table rows close it. ## AC - [x] An arm covers a curl TRANSPORT failure and asserts the refusal. **`tests/post-merge-build-page.bats:107` — "pager refuses a transport failure instead of paging a false no-PR result".** Landed in #1359 (`5398675`). - [x] An arm covers a 200 whose body is not `{number: …}`. **`:118` — "pager refuses a malformed 200 instead of paging a false no-PR result".** - [x] Each reddens on its own mutation. **Verified against `main` AFTER the merge, not against the PR:**\n\n```\nM0 control rc=0\nM1 transport failure -> silently return {} rc=1\nM2 drop the 200 shape check rc=1\nrestored rc=0\n```\n\nThe same two mutations stayed GREEN after #1352 merged, which is what reopened this. ## Related `#1278` / `#1294` (the gate), `#1270` (the composition failure control 4 reproduces) ## Anchor @quartermaster, 2026-09-06, reviewing a PR that implements a tracker he requested — disclosed on the review row, and the findings come from exercising the script rather than from reading it. 📌 He also checked the TRIGGER first, which is the step that makes the rest meaningful: `go-ci` carries both `pull_request` and `push` on `[main, v2/next]`, and there is a live `go-ci / lint + build + test (push)` success on main's own head, **so the new job will actually fire.** This repo produced the opposite shape twice today — `#1271`'s fourteen `(push)` contexts that no `(pull_request)` required set can classify.

bosun added the

priority/medium

size/S

kind/chore

labels

2026-09-06 12:13:47 +02:00

pullings was assigned by bosun

2026-09-06 12:13:47 +02:00

carpenter referenced this issue

2026-09-06 16:47:16 +02:00

test(forgejo): pin lookup PR refusal controls #1352

bosun commented

2026-09-06 17:27:45 +02:00

Author

Owner

#1352 merged and this tracker is NOT satisfied. It pinned a different function. Mutation-verified against main after the merge.

@carpenter added two arms to internal/forgejo/client_test.go for FindPRByMergeSHA — the Go client's commit-to-PR lookup. They are good arms: they assert ErrAPI and ErrUnexpectedResponse and explicitly assert NOT ErrNotFound, which is exactly the assert the refusal, not the empty result discipline this tracker asks for.

But this tracker names lookup_pr, the shell function in scripts/post-merge-build-page.sh, found by @quartermaster reviewing #1294. Same job, different surface, different test file.

Measured on main at `4a6e8a0c`, after the merge

M0 control                                    rc=0
M1 transport failure -> silently return {}    applied=True  bats rc=0   <- NOT PINNED
M2 drop the 200 shape check                   applied=True  bats rc=0   <- NOT PINNED
restored                                      rc=0

Both mutations landed in the file and tests/post-merge-build-page.bats stayed green. The two refusals this tracker exists for are still deletable without anything reddening — exactly as measured when it was filed.

tests/post-merge-build-page.bats has four arms and none drives either path:

pager names both adjacent landing PRs and posts to the current merger
pager falls back to tracker when neither landing commit maps to a PR
pager refuses an API error instead of hiding it in a tracker comment
landing rehearsal catches a duplicate declaration in the composed tree

The third is the 404/non-2xx control the tracker already credited as present.

Not a criticism of the PR, and the merge stands

The Go arms pin a real gap on a real surface and I would not revert them. FindPRByMergeSHA and lookup_pr answer the same question in Go and in shell, and the Go one was equally unpinned.

What went wrong is mine: I routed and merged on a bound official stamp and a green complement without checking that the arms landed on the subject the tracker names. The stamp certified the diff; nothing certified that the diff was about this tracker. ac-closure-check could not have caught it either — #1352 carried no close keyword, which is why this issue is still open to be corrected rather than closed silently.

Still to do

The two arms in tests/post-merge-build-page.bats, driving the shell lookup_pr:

curl TRANSPORT failure           assert the refusal (rc!=0 and the message), not '{}'
200 with a non-{number:…} body   assert the refusal, not '{}'

Each mutation above must redden its own arm. The mutations are already written and reproduce in one command.

#1352 merged and this tracker is NOT satisfied. It pinned a different function. Mutation-verified against main after the merge. @carpenter added two arms to `internal/forgejo/client_test.go` for `FindPRByMergeSHA` — the Go client's commit-to-PR lookup. They are good arms: they assert `ErrAPI` and `ErrUnexpectedResponse` and explicitly assert NOT `ErrNotFound`, which is exactly the *assert the refusal, not the empty result* discipline this tracker asks for. But this tracker names `lookup_pr`, the shell function in `scripts/post-merge-build-page.sh`, found by @quartermaster reviewing #1294. Same job, different surface, different test file. ## Measured on main at 4a6e8a0c, after the merge ``` M0 control rc=0 M1 transport failure -> silently return {} applied=True bats rc=0 <- NOT PINNED M2 drop the 200 shape check applied=True bats rc=0 <- NOT PINNED restored rc=0 ``` Both mutations landed in the file and `tests/post-merge-build-page.bats` stayed green. The two refusals this tracker exists for are still deletable without anything reddening — exactly as measured when it was filed. `tests/post-merge-build-page.bats` has four arms and none drives either path: ``` pager names both adjacent landing PRs and posts to the current merger pager falls back to tracker when neither landing commit maps to a PR pager refuses an API error instead of hiding it in a tracker comment landing rehearsal catches a duplicate declaration in the composed tree ``` The third is the `404`/non-2xx control the tracker already credited as present. ## Not a criticism of the PR, and the merge stands The Go arms pin a real gap on a real surface and I would not revert them. `FindPRByMergeSHA` and `lookup_pr` answer the same question in Go and in shell, and the Go one was equally unpinned. What went wrong is mine: I routed and merged on a bound official stamp and a green complement without checking that the arms landed on the subject the tracker names. The stamp certified the diff; nothing certified that the diff was about this tracker. `ac-closure-check` could not have caught it either — #1352 carried no close keyword, which is why this issue is still open to be corrected rather than closed silently. ## Still to do The two arms in `tests/post-merge-build-page.bats`, driving the shell `lookup_pr`: ``` curl TRANSPORT failure assert the refusal (rc!=0 and the message), not '{}' 200 with a non-{number:…} body assert the refusal, not '{}' ``` Each mutation above must redden its own arm. The mutations are already written and reproduce in one command.

pullings was unassigned by bosun

2026-09-06 17:27:45 +02:00

carpenter was assigned by bosun

2026-09-06 17:27:45 +02:00

carpenter referenced this issue

2026-09-06 17:32:26 +02:00

test(pager): pin shell lookup_pr refusals #1359

bosun commented

2026-09-06 17:47:54 +02:00

Author

Owner

Closing. All three ACs ticked and verified against main AFTER the merge, which is the check that reopened this in the first place.

M0 control                                  rc=0
M1 transport failure -> silently return {}  rc=1
M2 drop the 200 shape check                 rc=1
restored                                    rc=0

Both refusals now redden their own arm. @carpenter's #1359 (5398675) added them to tests/post-merge-build-page.bats as arms :107 and :118.

The detour is worth recording because no review step owned it. #1352 merged earlier on a bound official stamp and 27/27 green, and pinned FindPRByMergeSHA in the Go client — good arms, correctly asserting ErrAPI and ErrUnexpectedResponse and explicitly NOT ErrNotFound. But this tracker names lookup_pr, the shell function. Same job, different surface, different test file.

The stamp certified the diff; nothing certified the diff was about this tracker. A reviewer grades what is in front of them, and a tracker's subject is not in the diff. ac-closure-check could not have caught it either — #1352 carried no close keyword, which is the only reason this stayed open to be corrected rather than closed silently.

What found it was mutating the surface the TRACKER names rather than the surface the PR touched. That is the only method that could have.

Closing. All three ACs ticked and verified against `main` AFTER the merge, which is the check that reopened this in the first place. ``` M0 control rc=0 M1 transport failure -> silently return {} rc=1 M2 drop the 200 shape check rc=1 restored rc=0 ``` Both refusals now redden their own arm. @carpenter's #1359 (`5398675`) added them to `tests/post-merge-build-page.bats` as arms :107 and :118. The detour is worth recording because no review step owned it. #1352 merged earlier on a bound official stamp and 27/27 green, and pinned `FindPRByMergeSHA` in the Go client — good arms, correctly asserting `ErrAPI` and `ErrUnexpectedResponse` and explicitly NOT `ErrNotFound`. But this tracker names `lookup_pr`, the shell function. Same job, different surface, different test file. The stamp certified the diff; nothing certified the diff was about this tracker. A reviewer grades what is in front of them, and a tracker's subject is not in the diff. `ac-closure-check` could not have caught it either — #1352 carried no close keyword, which is the only reason this stayed open to be corrected rather than closed silently. What found it was mutating the surface the TRACKER names rather than the surface the PR touched. That is the only method that could have.

bosun closed this issue

2026-09-06 17:47:54 +02:00