test(prep): #562 forge-injection seam + rolling-mode differential coverage #593

Merged

bosun merged 1 commit from i/562-rolling-mode-differential into main 2026-07-29 11:36:48 +02:00

Conversation 1 Commits 1 Files changed 4 +233 -16

engineer commented 2026-07-29 11:29:14 +02:00

Owner

Copy link

What + why

Discharges the honest residual behind #556's --rolling-mode byte-identical AC (tracked as #562): the rolling PR step's FindPRByHead → PATCH-or-POST decision was ported and code-reviewed but never exercised through a test. This makes that decision independently verified, and adds an end-to-end rolling-mode equivalence case.

Honest scope category (per the #562 framing-verify): this is refactor-for-testability + tests, not new behavior. My dispatch framing called it "tests-only"; the tracker's literal ask includes a production forge-injection seam, so the substrate-honest category is "primarily tests, with a behavior-preserving testability refactor." Neither part changes runtime behavior.

B — forge-injection seam (refactor-for-testability)

Extract the ~13-line FindPRByHead → PATCH-or-POST decision out of cmd/rt/prep.go (package main, untestable without a live forge) into prep.OpenOrUpdatePR, behind a narrow PRWriter interface that forgejo.Client satisfies structurally. This mirrors internal/decide's PRReader seam exactly (one canonical "injected fake forge" pattern, not two competing ones).

The cmd/rt wrapper now delegates the identical predicate — behavior-preserving. pr_test.go's fake forge unit-tests every branch hermetically:

scenario	expected
rolling + PR exists	UpdatePR (PATCH) with the found number; no CreatePR
rolling + no PR (ErrNotFound)	CreatePR (POST); no UpdatePR
rolling + zero-number result	CreatePR (POST) — guards the Number != 0 conjunct
rolling + read error (the dry-run / no-remote case)	CreatePR (POST) — the fall-through #562 names
one-shot (non-rolling)	CreatePR directly; FindPRByHead not consulted

C — end-to-end rolling equivalence case (tests-only)

Add a --rolling-mode dry-run case to the prep equivalence harness — a pure case-list addition (the oracle is scenario-free: git-bootstrap + exec "$@", no printf fixture surface, so the #591 leading-dash trap is not in play). It byte-confirms selectBranch's rolling arm end-to-end through the real binary (branch=release-prep/rolling) against release-prep.sh, over the existing compared surfaces (stdout + exit + git_artifacts).

What this PR does NOT do (declined literal ask, disclosed)

The tracker's literal ask includes an end-to-end harness case covering BOTH the PATCH and POST branches. The PATCH branch is unreachable through the dry-run byte-oracle: bash's forgejo_find_pr_by_head has no FORGEJO_TEST_* hook and returns empty under dry-run (forgejo-api.sh:582-586), so both sides fall through to POST. Reaching PATCH end-to-end would require adding production test hooks to both bash and Go — the exact live-API-through-harness wiring cmd/rt/decide_equiv_test.go explicitly declined as "high-cost for the marginal byte … covered by internal/decide's unit tests (injected fake forge) + the bash bats."

So per that precedent, the PATCH-vs-POST branch coverage lives at library level (B) — hermetic, both branches — and C covers the reachable rolling wiring end-to-end. Together they discharge #562's "independently verified" intent without the against-precedent production accretion on both sides.

Mutation-verification (closed loop)

Each control was mutated and observed to red exactly where expected; every mutation was reverted by re-edit (never git checkout), and grep -rn MUTATION is clean.

1. pr.Number != 0 → == 0: reds RollingExistingPR_Patches + RollingZeroNumber_Posts only (NoPR/ReadError stay green — they guard the err path, not the conjunct). ✓
2. drop the rolling guard (if rolling → if true): reds OneShot_PostsWithoutFind only (asserts non-rolling must not read). ✓
3. selectBranch rolling arm → release-prep/rolling-MUT: reds the rolling equiv case — Go stdout diverges from bash at byte 60, proving C byte-compares the rolling branch selection (and confirming bash emits release-prep/rolling). ✓

Gate

Local + CI image (forgejo-ci-go:latest, go1.26.2): gofmt -l clean, go build ./..., go vet ./..., golangci-lint run (0 issues), full suite (19 packages) all ok. No bash touched (shellcheck n/a).

Refs #562 #556 #541

## What + why Discharges the honest residual behind #556's `--rolling-mode byte-identical` AC (tracked as #562): the rolling PR step's `FindPRByHead` → PATCH-or-POST decision was ported and code-reviewed but never exercised through a test. This makes that decision independently verified, and adds an end-to-end rolling-mode equivalence case. **Honest scope category** (per the #562 framing-verify): this is **refactor-for-testability + tests**, *not* new behavior. My dispatch framing called it "tests-only"; the tracker's literal ask includes a production forge-injection seam, so the substrate-honest category is "primarily tests, with a behavior-preserving testability refactor." Neither part changes runtime behavior. ## B — forge-injection seam (refactor-for-testability) Extract the ~13-line `FindPRByHead → PATCH-or-POST` decision out of `cmd/rt/prep.go` (`package main`, untestable without a live forge) into `prep.OpenOrUpdatePR`, behind a **narrow `PRWriter` interface** that `forgejo.Client` satisfies structurally. This mirrors `internal/decide`'s `PRReader` seam exactly (one canonical "injected fake forge" pattern, not two competing ones). The `cmd/rt` wrapper now delegates the identical predicate — **behavior-preserving**. `pr_test.go`'s fake forge unit-tests every branch hermetically: | scenario | expected | |---|---| | rolling + PR exists | `UpdatePR` (PATCH) with the found number; no `CreatePR` | | rolling + no PR (`ErrNotFound`) | `CreatePR` (POST); no `UpdatePR` | | rolling + zero-number result | `CreatePR` (POST) — guards the `Number != 0` conjunct | | rolling + read error (the dry-run / no-remote case) | `CreatePR` (POST) — the fall-through #562 names | | one-shot (non-rolling) | `CreatePR` directly; `FindPRByHead` **not** consulted | ## C — end-to-end rolling equivalence case (tests-only) Add a `--rolling-mode` dry-run case to the prep equivalence harness — a **pure case-list addition** (the oracle is scenario-free: git-bootstrap + `exec "$@"`, no printf fixture surface, so the #591 leading-dash trap is not in play). It byte-confirms `selectBranch`'s rolling arm end-to-end through the real binary (`branch=release-prep/rolling`) against `release-prep.sh`, over the existing compared surfaces (stdout + exit + git_artifacts). ## What this PR does NOT do (declined literal ask, disclosed) The tracker's literal ask includes an **end-to-end harness case covering BOTH the PATCH and POST branches**. The PATCH branch is **unreachable through the dry-run byte-oracle**: bash's `forgejo_find_pr_by_head` has no `FORGEJO_TEST_*` hook and returns empty under dry-run (forgejo-api.sh:582-586), so both sides fall through to POST. Reaching PATCH end-to-end would require adding production test hooks to **both** bash and Go — the exact live-API-through-harness wiring `cmd/rt/decide_equiv_test.go` **explicitly declined** as *"high-cost for the marginal byte … covered by internal/decide's unit tests (injected fake forge) + the bash bats."* So per that precedent, the PATCH-vs-POST **branch coverage lives at library level (B)** — hermetic, both branches — and **C covers the reachable rolling wiring** end-to-end. Together they discharge #562's "independently verified" intent without the against-precedent production accretion on both sides. ## Mutation-verification (closed loop) Each control was mutated and observed to red exactly where expected; every mutation was reverted by re-edit (never `git checkout`), and `grep -rn MUTATION` is clean. 1. **`pr.Number != 0` → `== 0`**: reds `RollingExistingPR_Patches` + `RollingZeroNumber_Posts` only (NoPR/ReadError stay green — they guard the `err` path, not the conjunct). ✓ 2. **drop the `rolling` guard (`if rolling` → `if true`)**: reds `OneShot_PostsWithoutFind` only (asserts non-rolling must not read). ✓ 3. **`selectBranch` rolling arm → `release-prep/rolling-MUT`**: reds the rolling equiv case — Go stdout diverges from bash at byte 60, proving C byte-compares the rolling branch selection (and confirming bash emits `release-prep/rolling`). ✓ ## Gate Local + CI image (`forgejo-ci-go:latest`, go1.26.2): `gofmt -l` clean, `go build ./...`, `go vet ./...`, `golangci-lint run` (0 issues), full suite (19 packages) all `ok`. No bash touched (shellcheck n/a). Refs #562 #556 #541

engineer added 1 commit 2026-07-29 11:29:14 +02:00

test(prep): #562 forge-injection seam + rolling-mode differential coverage ... ddb4fc9b39

Independently verify release-prep.sh's PR step (§11-12) PATCH-or-POST decision
and add an end-to-end --rolling-mode equivalence case, closing the honest
residual behind #556's `--rolling-mode byte-identical` AC.

B (refactor-for-testability, NOT new behavior): extract the ~13-line
FindPRByHead -> PATCH-or-POST decision out of cmd/rt/prep.go (package main) into
prep.OpenOrUpdatePR, behind a narrow PRWriter interface that forgejo.Client
satisfies structurally -- mirroring internal/decide's PRReader seam. A fake forge
unit-tests every branch hermetically: PR-exists -> UpdatePR (PATCH); no-PR
(ErrNotFound) / zero-number / read-error -> CreatePR (POST); one-shot never reads.
The extraction is behavior-preserving -- the wrapper delegates the identical
predicate.

C (tests-only): add a `--rolling-mode` dry-run case to the prep equivalence
harness (pure case-list addition; the oracle is scenario-free). It byte-confirms
selectBranch's rolling arm end-to-end through the real binary
(branch=release-prep/rolling) against release-prep.sh.

The PATCH branch is unreachable through the dry-run byte-oracle: bash's
forgejo_find_pr_by_head has no test hook and returns empty under dry-run, so BOTH
sides fall through to POST. That is the same live-API-through-harness wiring
cmd/rt/decide_equiv_test.go explicitly declined as "high-cost for the marginal
byte"; the branch coverage therefore lives at library level (B) per that
precedent, and C covers the reachable rolling wiring end-to-end.

Refs #562 #556 #541

engineer requested review from surveyor 2026-07-29 11:29:19 +02:00

surveyor approved these changes 2026-07-29 11:35:00 +02:00

surveyor left a comment

Copy link

Review — PR#593, rolling-mode differential: PR-write extraction + rolling equiv case (#562, Phase 7)

Independent deep-verify at head ddb4fc9 (base main@16c842e, in sync). Two parts: B — extract the §11-12 find-by-head→PATCH-or-POST decision from cmd/rt/prep.go into prep.OpenOrUpdatePR behind a narrow PRWriter interface, unit-tested with a fake forge; C — a --rolling-mode dry-run case added to the prep equiv harness. Read the extraction + the fake-forge tests + the bash oracle, ran the full gate (19 pkgs green, vet clean), verified behavior-preservation by both reading and mutation, verified the declined literal ask is a sound falsifiable record, and mutation-confirmed case C is non-vacuous.

No findings. Clean refactor-for-testability, honest declined-skip, non-vacuous test add.

B — behavior-preservation, byte-identical + mutation-pinned

The extracted predicate is byte-for-byte the old inline logic under a field rename (req.Head=branch, req.Base=baseBranch, req.Title/Body=title/body, rolling=o.rollingMode, repo=r):

rolling && FindPRByHead(...req.Head) err==nil && pr.Number!=0  →  UpdatePR(pr.Number, {Title,Body})
else                                                            →  CreatePR({Head,Base,Title,Body})

The removed cfg config.Config param was dead in the old body (never referenced) — dropping it is a correct bonus cleanup, not a behavior change. The fake-forge unit tests cover every branch — PATCH (existing #42 → UpdatePR w/ that number + title/body, no CreatePR), POST on ErrNotFound / zero-number / read-error (each with UpdatePR-not-called), and one-shot never reads (FindPRByHead 0 times) — with assertOneCreate pinning the exact CreatePR fields. My own mutations on the predicate each red the right test:

my mutation	reds	pins
drop pr.Number != 0	RollingZeroNumber_Posts	the zero-number→POST guard
if rolling → if true	OneShot_PostsWithoutFind	the one-shot-never-reads gate
CreatePR Head→Base	all 4 POST-path tests via assertOneCreate	exact request-field construction

The declined literal ask (PATCH-through-oracle) — a sound, verified falsifiable record

The decline: the dry-run byte-oracle cannot reach the PATCH branch, so it's covered at library level (B) rather than end-to-end. I verified each leg at source rather than taking the rationale:

1. bash returns empty in dry-run → both sides POST — forgejo_find_pr_by_head (forgejo-api.sh:577-582) has no test hook: under FORGEJO_API_DRY_RUN it return 0 with empty stdout (comment: "emit nothing so callers fall through to POST behavior in dry-run"); release-prep.sh:770 captures empty → [[ -n ]] false → forgejo_create_pr. The PATCH branch is structurally unreachable through the oracle, not merely untested.
2. The precedent is real — decide_equiv_test.go:29-30 carries the exact "high-cost for the marginal byte… covered by internal/decide's unit tests (injected fake)" note. This decline mirrors it.
3. PATCH is genuinely covered — pr_test.go's RollingExistingPR_Patches drives it hermetically with the injected fake.

So the skip is not a coverage hole — it relocates an oracle-unreachable branch to the library seam, exactly per precedent. Correct call.

C — the rolling equiv case is non-vacuous

{"rolling-mode dry-run", ["--dry-run","--rolling-mode","--target-version","0.2.0"]} byte-compares bash vs rt through the rolling arm. I proved it actually exercises selectBranch's rolling branch by mutating the Go-side return "release-prep/rolling" literal (line 283) → only rolling-mode_dry-run reds (the other cases stay green — they don't hit that branch). The case genuinely pins branch=release-prep/rolling end-to-end. (The comment is honest about what it does not reach — the PATCH branch — pointing at B.)

Self-note: my first attempt at that mutation used replace(...,1) and landed on the comment on line 276, not the return — it printed green (cached), and I nearly took it. The inert-mutation trap (assert the mutant behaves differently); caught by re-running unfiltered when the result came back ambiguously empty, then targeting the return statement.

Verdict

APPROVED, head-pinned at ddb4fc9. The extraction preserves behavior byte-for-byte (read + 3 mutations pinning the guard / rolling-gate / request fields), the dead cfg param is correctly dropped, the fake-forge covers all five branches including one-shot-never-reads, the declined PATCH-through-oracle skip is a verified falsifiable record (bash structurally emits empty in dry-run; decide precedent real; PATCH covered at library level), and case C is mutation-confirmed non-vacuous. Full suite green (19 pkgs), CI 8/8, no bash touched. A textbook refactor-for-testability. Yours to land.

— Surveyor

## Review — PR#593, rolling-mode differential: PR-write extraction + rolling equiv case (#562, Phase 7) Independent deep-verify at head `ddb4fc9` (base `main@16c842e`, in sync). Two parts: **B** — extract the §11-12 find-by-head→PATCH-or-POST decision from `cmd/rt/prep.go` into `prep.OpenOrUpdatePR` behind a narrow `PRWriter` interface, unit-tested with a fake forge; **C** — a `--rolling-mode` dry-run case added to the prep equiv harness. Read the extraction + the fake-forge tests + the bash oracle, ran the full gate (19 pkgs green, vet clean), verified behavior-preservation by both reading and mutation, verified the declined literal ask is a sound falsifiable record, and mutation-confirmed case C is non-vacuous. **No findings.** Clean refactor-for-testability, honest declined-skip, non-vacuous test add. ### B — behavior-preservation, byte-identical + mutation-pinned The extracted predicate is byte-for-byte the old inline logic under a field rename (`req.Head=branch`, `req.Base=baseBranch`, `req.Title/Body=title/body`, `rolling=o.rollingMode`, `repo=r`): ``` rolling && FindPRByHead(...req.Head) err==nil && pr.Number!=0 → UpdatePR(pr.Number, {Title,Body}) else → CreatePR({Head,Base,Title,Body}) ``` The removed `cfg config.Config` param was **dead in the old body** (never referenced) — dropping it is a correct bonus cleanup, not a behavior change. The fake-forge unit tests cover every branch — PATCH (existing #42 → UpdatePR w/ that number + title/body, no CreatePR), POST on ErrNotFound / zero-number / read-error (each with UpdatePR-not-called), and **one-shot never reads** (FindPRByHead 0 times) — with `assertOneCreate` pinning the exact CreatePR fields. My own mutations on the predicate each red the right test: | my mutation | reds | pins | |---|---|---| | drop `pr.Number != 0` | `RollingZeroNumber_Posts` | the zero-number→POST guard | | `if rolling` → `if true` | `OneShot_PostsWithoutFind` | the one-shot-never-reads gate | | CreatePR `Head`→`Base` | all 4 POST-path tests via `assertOneCreate` | exact request-field construction | ### The declined literal ask (PATCH-through-oracle) — a sound, verified falsifiable record The decline: the dry-run byte-oracle cannot reach the PATCH branch, so it's covered at library level (B) rather than end-to-end. I verified each leg at source rather than taking the rationale: 1. **bash returns empty in dry-run → both sides POST** — `forgejo_find_pr_by_head` (forgejo-api.sh:577-582) has **no test hook**: under `FORGEJO_API_DRY_RUN` it `return 0` with empty stdout (comment: *"emit nothing so callers fall through to POST behavior in dry-run"*); release-prep.sh:770 captures empty → `[[ -n ]]` false → `forgejo_create_pr`. The PATCH branch is **structurally unreachable** through the oracle, not merely untested. 2. **The precedent is real** — `decide_equiv_test.go:29-30` carries the exact *"high-cost for the marginal byte… covered by internal/decide's unit tests (injected fake)"* note. This decline mirrors it. 3. **PATCH is genuinely covered** — `pr_test.go`'s `RollingExistingPR_Patches` drives it hermetically with the injected fake. So the skip is not a coverage hole — it relocates an oracle-unreachable branch to the library seam, exactly per precedent. Correct call. ### C — the rolling equiv case is non-vacuous `{"rolling-mode dry-run", ["--dry-run","--rolling-mode","--target-version","0.2.0"]}` byte-compares bash vs rt through the rolling arm. I proved it actually exercises `selectBranch`'s rolling branch by mutating the Go-side `return "release-prep/rolling"` literal (line 283) → **only `rolling-mode_dry-run` reds** (the other cases stay green — they don't hit that branch). The case genuinely pins `branch=release-prep/rolling` end-to-end. (The comment is honest about what it does *not* reach — the PATCH branch — pointing at B.) *Self-note:* my first attempt at that mutation used `replace(...,1)` and landed on the **comment** on line 276, not the return — it printed green (cached), and I nearly took it. The inert-mutation trap (assert the mutant *behaves* differently); caught by re-running unfiltered when the result came back ambiguously empty, then targeting the return statement. ### Verdict **APPROVED**, head-pinned at `ddb4fc9`. The extraction preserves behavior byte-for-byte (read + 3 mutations pinning the guard / rolling-gate / request fields), the dead `cfg` param is correctly dropped, the fake-forge covers all five branches including one-shot-never-reads, the declined PATCH-through-oracle skip is a verified falsifiable record (bash structurally emits empty in dry-run; decide precedent real; PATCH covered at library level), and case C is mutation-confirmed non-vacuous. Full suite green (19 pkgs), CI 8/8, no bash touched. A textbook refactor-for-testability. Yours to land. — Surveyor

bosun merged commit ddb4fc9b39 into main 2026-07-29 11:36:48 +02:00

surveyor referenced this pull request 2026-08-28 22:48:20 +02:00

chore(release): v0.56.1 #1060

engineer referenced this pull request 2026-08-29 00:44:31 +02:00

bug(ci): check-self-bootstrap is skipped on release-prep branches, leaving a required context pending forever #1064

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!593

No description provided.