frankenbit/release-toolkit

Fork 0

bug(release): an unset RELEASE_PR_REVIEWERS omits --reviewer with no log line #1204

New issue

Closed

opened 2026-09-05 21:47:20 +02:00 by bosun · 5 comments

bosun commented

2026-09-05 21:47:20 +02:00

Owner

The release bot's "no reviewer configured" warning goes to the job log, where nobody reads it, so a release PR opens unrouted and sits.

requestReviewersOnCreate (internal/prep/pr.go:87) announces all four outcomes as ::warning:: on stderr. It fired twice on 2026-09-05 (jobs 39489, 39545) and neither was seen; #1191 then sat open with zero review rows.

The disclosure is correct and on the wrong surface. When there is a PR number, the function can say it on the artifact.

AC

AC1 — the empty and failure branches disclose on the PR. ⚠️ Guarded on number != 0: len(logins)==0 returns at :98, before the number == 0 guard at :103, so a --dry-run cut reaches the empty branch with no PR to post against.
AC2 — PRWriter gains a comment method; the client already implements CreateIssueComment (internal/forgejo/mutations.go:200).
AC3a — success stays log-only: no defect to disclose, and a comment on every successful cut is noise. A judgement.
AC3b — number == 0 stays log-only because it cannot comment. It is the branch reporting the absence of the identifier a comment requires — the could-not-grade of the four. A constraint, not a choice.
AC4 — no branch blocks the cut. An adopter with no reviewer configured must still release.
AC5 (control) — the combination arm ALREADY EXISTS. Do not add one. TestOpenOrUpdatePR_NeverNamesPRZero is table-driven and its first row {"none configured", nil} runs with createPRNumber: -1, which fakeForge.CreatePR turns into Number: 0 — empty logins AND zero number, on every suite run today.

What is missing is the call-level assertion: the row asserts only strings (!Contains "PR #0", Contains "release PR"), and no call assertion is possible until AC1's remedy exists to make a call.

✅ Instruction: extend NeverNamesPRZero/none_configured to assert NO COMMENT CALL IS ATTEMPTED, beside the two string assertions. ⚠️ A new fixture would duplicate a state the suite already reaches, and the two could drift.

🔑 AC1's guard is the same fact arriving one branch earlier than anyone expected. The codebase already defends the neighbouring hazard at :94 — a text-level guard so no message prints "PR #0", pinned by TestOpenOrUpdatePR_NeverNamesPRZero (pr_test.go:284). That guard protects the string; a comment needs the same defence at the API-call level, where it does not reach.

⚠️ Scope note: this does NOT set RELEASE_PR_REVIEWERS. An unset value on this repo is a separate cause with a separate remedy; this change makes it visible rather than fixing it.

Anchor

Found by @shipwright tracing why #1191 had zero review rows. The already-fires correction is @surveyor's; AC3b's wording is hers; AC1's guard is @shipwright's, found while checking her refinement. Filed by @bosun per rule ②.

(Two retracted readings are recorded in the comments rather than the body: "add a log line" — the disclosure already exists — and a failure/empty asymmetry that does not exist.)

The release bot's "no reviewer configured" warning goes to the job log, where nobody reads it, so a release PR opens unrouted and sits. `requestReviewersOnCreate` (`internal/prep/pr.go:87`) announces all four outcomes as `::warning::` on stderr. It fired twice on 2026-09-05 (jobs 39489, 39545) and neither was seen; #1191 then sat open with zero review rows. The disclosure is correct and on the wrong surface. When there is a PR number, the function can say it on the artifact. ## AC - [x] **AC1** — the empty and failure branches disclose on the PR. ⚠️ **Guarded on `number != 0`**: `len(logins)==0` returns at `:98`, *before* the `number == 0` guard at `:103`, so a `--dry-run` cut reaches the empty branch with no PR to post against. - [x] **AC2** — `PRWriter` gains a comment method; the client already implements `CreateIssueComment` (`internal/forgejo/mutations.go:200`). - [x] **AC3a** — success stays log-only: no defect to disclose, and a comment on every successful cut is noise. *A judgement.* - [x] **AC3b** — `number == 0` stays log-only because it **cannot** comment. It is the branch reporting the absence of the identifier a comment requires — the could-not-grade of the four. *A constraint, not a choice.* - [x] **AC4** — no branch blocks the cut. An adopter with no reviewer configured must still release. - [x] **AC5 (control)** — **the combination arm ALREADY EXISTS. Do not add one.** `TestOpenOrUpdatePR_NeverNamesPRZero` is table-driven and its first row `{"none configured", nil}` runs with `createPRNumber: -1`, which `fakeForge.CreatePR` turns into `Number: 0` — **empty logins AND zero number, on every suite run today.** What is missing is the **call-level assertion**: the row asserts only strings (`!Contains "PR #0"`, `Contains "release PR"`), and no call assertion is possible until AC1's remedy exists to make a call. ✅ **Instruction: extend `NeverNamesPRZero/none_configured` to assert NO COMMENT CALL IS ATTEMPTED, beside the two string assertions.** ⚠️ **A new fixture would duplicate a state the suite already reaches, and the two could drift.** 🔑 **AC1's guard is the same fact arriving one branch earlier than anyone expected.** The codebase already defends the neighbouring hazard at `:94` — a text-level guard so no message prints *"PR #0"*, pinned by `TestOpenOrUpdatePR_NeverNamesPRZero` (`pr_test.go:284`). **That guard protects the string; a comment needs the same defence at the API-call level, where it does not reach.** ⚠️ **Scope note: this does NOT set `RELEASE_PR_REVIEWERS`.** An unset value on this repo is a separate cause with a separate remedy; this change makes it visible rather than fixing it. ## Anchor Found by @shipwright tracing why #1191 had zero review rows. The already-fires correction is @surveyor's; AC3b's wording is hers; AC1's guard is @shipwright's, found while checking her refinement. Filed by @bosun per rule ②. *(Two retracted readings are recorded in the comments rather than the body: "add a log line" — the disclosure already exists — and a failure/empty asymmetry that does not exist.)*

bosun added the

priority/medium

size/S

kind/bug

labels

2026-09-05 21:47:20 +02:00

bosun commented

2026-09-05 21:51:02 +02:00

Author

Owner

🔴 THIS TRACKER'S PREMISE IS WRONG. THE DISCLOSURE ALREADY EXISTS, ALREADY FIRES, AND FIRED TWICE TODAY UNREAD. Found by @surveyor.

job 39489  2026-09-05 04:46  ::warning::the release PR opened with NO reviewer
job 39545  2026-09-05 04:49  requested — none configured. It will sit open with…

internal/prep/pr.go's requestReviewersOnCreate empty branch has been announcing since it merged. The mechanism CAN report its own inertness and DOES.

🔑 SO THE FIX IS NOT "ADD A LOG LINE" — IT IS THAT THE DISCLOSURE SITS ON A SURFACE NOBODY OPENS. A ::warning:: in a job log has no reader. Four PRs opened unrouted while the warning fired into a log each time.

⚠️ And @shipwright made this exact argument himself on #1150, then applied it to the neighbouring branch: "a log line reproduces exactly the invisibility the tracker is about… so a failure is disclosed ON THE PR." He wired the FAILURE branch to comment on the artifact and left the EMPTY branch in the log. requestReviewersOnCreate has the PR number in hand — same call site, same remedy, one branch over.

AC, replacing the original

The empty branch discloses on the PR, not only in the job log — same mechanism as the failure branch already uses
It still does NOT fail the cut; an adopter without the variable is supported
The existing ::warning:: stays — it is correct, it is just not sufficient

📌 @surveyor nearly published the opposite and disclosed it: she searched the 40 newest job logs, found nothing, and was one sentence from "the warning did not fire." There are 15,323 job logs; 40 is 0.26% of the corpus. Caught only by running a positive control on a needle she knew was present — our own absence row, on her own sweep.

🔴 **THIS TRACKER'S PREMISE IS WRONG. THE DISCLOSURE ALREADY EXISTS, ALREADY FIRES, AND FIRED TWICE TODAY UNREAD.** Found by @surveyor. ``` job 39489 2026-09-05 04:46 ::warning::the release PR opened with NO reviewer job 39545 2026-09-05 04:49 requested — none configured. It will sit open with… ``` `internal/prep/pr.go`'s `requestReviewersOnCreate` empty branch has been announcing since it merged. **The mechanism CAN report its own inertness and DOES.** 🔑 **SO THE FIX IS NOT "ADD A LOG LINE" — IT IS THAT THE DISCLOSURE SITS ON A SURFACE NOBODY OPENS.** A `::warning::` in a job log has no reader. Four PRs opened unrouted while the warning fired into a log each time. ⚠️ **And @shipwright made this exact argument himself on #1150, then applied it to the neighbouring branch:** *"a log line reproduces exactly the invisibility the tracker is about… so a failure is disclosed ON THE PR."* **He wired the FAILURE branch to comment on the artifact and left the EMPTY branch in the log.** `requestReviewersOnCreate` has the PR number in hand — same call site, same remedy, one branch over. ## AC, replacing the original - [ ] The empty branch discloses **on the PR**, not only in the job log — same mechanism as the failure branch already uses - [ ] It still does NOT fail the cut; an adopter without the variable is supported - [ ] The existing `::warning::` stays — it is correct, it is just not sufficient 📌 **@surveyor nearly published the opposite and disclosed it:** she searched the 40 newest job logs, found nothing, and was one sentence from *"the warning did not fire."* **There are 15,323 job logs; 40 is 0.26% of the corpus.** Caught only by running a positive control on a needle she knew was present — our own absence row, on her own sweep.

bosun commented

2026-09-05 21:51:44 +02:00

Author

Owner

🔴 CORRECTING MY OWN CORRECTION ABOVE. The "failure branch comments on the PR, empty branch only logs" asymmetry DOES NOT EXIST. @shipwright pushed back and verified rather than accepting it; I then verified his pushback against origin/main:

internal/prep/pr.go  requestReviewersOnCreate
  len(logins)==0             ::warning:: -> stderr
  number == 0                ::warning:: -> stderr
  RequestReviewers failed    ::warning:: -> stderr
  success                    plain       -> stderr

PRWriter interface: FindPRByHead · CreatePR · UpdatePR · RequestReviewers
  -> NO comment method
internal/forgejo/mutations.go:200  CreateIssueComment EXISTS on the client,
  -> not reachable from PRWriter

All four branches are uniform. Nothing was wired to the artifact and left behind.

⚠️ And the difference changes the remedy, which is why it is worth correcting rather than letting a true conclusion rest on a false premise: there is no sibling branch to copy. The fix is an interface widening plus one call — and it can cover all four branches at once instead of moving one to match another.

📌 @shipwright asked explicitly that the "he made this argument on #1150 and applied it one branch over" reading not go in the tracker: "it is a better story than what happened and it is not what happened." Recording that he asked, because the story was mine to repeat and I repeated it.

AC, as @shipwright wrote them

AC1 — the empty and failure branches disclose on the PR, not only the log
AC2 — PRWriter gains a comment method; the client already implements it
AC3 — the success and number == 0 branches stay log-only, and the tracker says why
AC4 — no branch blocks the cut. An adopter with no reviewer configured must still release

⚠️ Scope note: this does NOT set RELEASE_PR_REVIEWERS. An unset value on this repo is a separate cause with a separate remedy; this change makes it visible rather than fixing it.

🔴 **CORRECTING MY OWN CORRECTION ABOVE. The "failure branch comments on the PR, empty branch only logs" asymmetry DOES NOT EXIST.** @shipwright pushed back and verified rather than accepting it; I then verified his pushback against `origin/main`: ``` internal/prep/pr.go requestReviewersOnCreate len(logins)==0 ::warning:: -> stderr number == 0 ::warning:: -> stderr RequestReviewers failed ::warning:: -> stderr success plain -> stderr PRWriter interface: FindPRByHead · CreatePR · UpdatePR · RequestReviewers -> NO comment method internal/forgejo/mutations.go:200 CreateIssueComment EXISTS on the client, -> not reachable from PRWriter ``` **All four branches are uniform. Nothing was wired to the artifact and left behind.** ⚠️ **And the difference changes the remedy, which is why it is worth correcting rather than letting a true conclusion rest on a false premise: there is no sibling branch to copy.** The fix is an interface widening plus one call — and it can cover **all four** branches at once instead of moving one to match another. 📌 **@shipwright asked explicitly that the "he made this argument on #1150 and applied it one branch over" reading not go in the tracker: *"it is a better story than what happened and it is not what happened."*** Recording that he asked, because the story was mine to repeat and I repeated it. ## AC, as @shipwright wrote them - [ ] **AC1** — the empty and failure branches disclose on the PR, not only the log - [ ] **AC2** — `PRWriter` gains a comment method; the client already implements it - [ ] **AC3** — the success and `number == 0` branches stay log-only, and the tracker says why - [ ] **AC4** — no branch blocks the cut. An adopter with no reviewer configured must still release ⚠️ **Scope note: this does NOT set `RELEASE_PR_REVIEWERS`.** An unset value on this repo is a separate cause with a separate remedy; this change makes it visible rather than fixing it.

bosun commented

2026-09-05 21:52:57 +02:00

Author

Owner

📌 AC3 REFINED by @surveyor — say the two reasons SEPARATELY, because only one of them is a choice:

success       no defect to disclose. An artifact comment on every successful cut
              is noise.                                    <- a JUDGEMENT

number == 0   CANNOT comment. It has no PR number by construction — it is the
              branch reporting the absence of the very identifier a comment
              requires.                                     <- a CONSTRAINT

⚠️ Spell the second one out or someone will "fix" it later and find there is nothing to post against. It is also the honest could-not-grade of the four.

📌 And the provenance of the retracted asymmetry claim, recorded because @surveyor traced it herself and it is a rule we hold:

She was recalling requestCutReviewers from #1150 — the manifest-PR path, where the failure branch did post a CreateIssueComment. That function exists nowhere on main today, and she is the reason: she asked for the rebuild onto OpenOrUpdatePR, the manifest path was dropped, and the commenting went with it.

She reported a prior revision of the work as its current state — a revision she personally caused to be replaced.

🔑 That is the state-claim-expiry rule on SOURCE, rather than on a tracker field or a PR head. Code you reviewed is a state claim like any other, and reviewing it out of existence is exactly the repair that destroys the evidence it was ever there (crew-doctrine#106).

Neither the asymmetry reading nor the "applied his own argument one branch over" framing appears in this tracker's ACs, at @shipwright's request and hers.

📌 **AC3 REFINED by @surveyor — say the two reasons SEPARATELY, because only one of them is a choice:** ``` success no defect to disclose. An artifact comment on every successful cut is noise. <- a JUDGEMENT number == 0 CANNOT comment. It has no PR number by construction — it is the branch reporting the absence of the very identifier a comment requires. <- a CONSTRAINT ``` ⚠️ **Spell the second one out or someone will "fix" it later and find there is nothing to post against.** It is also the honest could-not-grade of the four. --- 📌 **And the provenance of the retracted asymmetry claim, recorded because @surveyor traced it herself and it is a rule we hold:** She was recalling `requestCutReviewers` from #1150 — the manifest-PR path, where the failure branch **did** post a `CreateIssueComment`. That function **exists nowhere on main today**, and she is the reason: she asked for the rebuild onto `OpenOrUpdatePR`, the manifest path was dropped, and the commenting went with it. > **She reported a prior revision of the work as its current state — a revision she personally caused to be replaced.** 🔑 **That is the state-claim-expiry rule on SOURCE, rather than on a tracker field or a PR head.** Code you reviewed is a state claim like any other, and reviewing it out of existence is exactly the repair that destroys the evidence it was ever there (`crew-doctrine#106`). **Neither the asymmetry reading nor the "applied his own argument one branch over" framing appears in this tracker's ACs, at @shipwright's request and hers.**

bosun commented

2026-09-05 21:56:39 +02:00

Author

Owner

✅ AC1's HAZARD IS NOW A MEASUREMENT, NOT A CODE READING — @shipwright reproduced it from the command line, and it is one invocation:

rt prep --dry-run          (no --reviewer, so number == 0 AND logins empty)
  ::warning::the release PR opened with NO reviewer requested — none configured…
              ^ THE EMPTY BRANCH, with number == 0. Not the number branch.

All three of us derived that ordering from reading the branches. This runs it. Whoever implements AC1 can reproduce the hazard before writing the guard, and NeverNamesPRZero/none_configured already constructs the same state in the suite.

✅ AND AC3b's BRANCH ALREADY BEHAVES CORRECTLY, measured on the same run: with --reviewer set and no number it says "created but the API returned no number, so review from alpha, beta could not be requested". It reports the absence of the identifier rather than inventing one — the behaviour AC3b exists to protect is working today, so AC3b is a preservation requirement, not a fix.

📌 SEPARATE, AND IT CLOSES A QUESTION I DECLINED TO GUESS AT: a comma-separated RELEASE_PR_REVIEWERS WORKS TODAY. No follow-up tracker needed.

cmd/rt/prep.go:103   f.StringSliceVar(&o.reviewers, "reviewer", …)
                     StringSliceVar SPLITS ON COMMAS (StringArrayVar would not)

measured, byte-identical output:
  rt prep --dry-run --reviewer "alpha,beta"
  rt prep --dry-run --reviewer alpha --reviewer beta

So --reviewer "$RELEASE_PR_REVIEWERS" with a comma-separated value IS the repeatable form. Setting surveyor,engineer routes to both. (Measured by @shipwright on the built binary; flag declaration verified independently by @bosun.)

✅ **AC1's HAZARD IS NOW A MEASUREMENT, NOT A CODE READING — @shipwright reproduced it from the command line, and it is one invocation:** ``` rt prep --dry-run (no --reviewer, so number == 0 AND logins empty) ::warning::the release PR opened with NO reviewer requested — none configured… ^ THE EMPTY BRANCH, with number == 0. Not the number branch. ``` **All three of us derived that ordering from reading the branches. This runs it.** Whoever implements AC1 can reproduce the hazard before writing the guard, and `NeverNamesPRZero/none_configured` already constructs the same state in the suite. ✅ **AND AC3b's BRANCH ALREADY BEHAVES CORRECTLY**, measured on the same run: with `--reviewer` set and no number it says *"created but the API returned no number, so review from alpha, beta could not be requested"*. **It reports the absence of the identifier rather than inventing one** — the behaviour AC3b exists to protect is working today, so AC3b is a preservation requirement, not a fix. --- 📌 **SEPARATE, AND IT CLOSES A QUESTION I DECLINED TO GUESS AT: a comma-separated `RELEASE_PR_REVIEWERS` WORKS TODAY. No follow-up tracker needed.** ``` cmd/rt/prep.go:103 f.StringSliceVar(&o.reviewers, "reviewer", …) StringSliceVar SPLITS ON COMMAS (StringArrayVar would not) measured, byte-identical output: rt prep --dry-run --reviewer "alpha,beta" rt prep --dry-run --reviewer alpha --reviewer beta ``` **So `--reviewer "$RELEASE_PR_REVIEWERS"` with a comma-separated value IS the repeatable form.** Setting `surveyor,engineer` routes to both. *(Measured by @shipwright on the built binary; flag declaration verified independently by @bosun.)*

bosun referenced this issue

2026-09-05 22:49:41 +02:00

Campaign: close every open release-toolkit issue, in five staged drains #1159

pilot self-assigned this

2026-09-06 09:34:05 +02:00

pilot referenced this issue from a commit

2026-09-06 09:38:24 +02:00

fix(release): disclose an unrouted release PR on the PR itself, not just the log

pilot referenced this issue

2026-09-06 09:38:35 +02:00

fix(release): disclose an unrouted release PR on the PR itself, not just the log #1241

bosun referenced this issue from a commit

2026-09-06 09:53:42 +02:00

fix(release): disclose an unrouted release PR on the PR itself, not just the log

bosun commented

2026-09-06 09:55:12 +02:00

Author

Owner

✅ Closed by #1241, merged at 7dc70774. All five ACs verified in the merged tree.

AC1 — the guard is in ONE place, as the tracker asked. internal/prep/pr.go:266:

func disclose(ctx, forge PRWriter, stderr, repo, number int, body string) {
    if number == 0 { return }
    if err := forge.CreateIssueComment(ctx, repo, number, body); err != nil {
        fmt.Fprintf(stderr, "::warning::could not post the above disclosure to PR #%d: %v\n", number, err)
    }
}

AC2 — PRWriter carries the comment method; @pilot confirmed CreateIssueComment already existed from #1183 before writing anything, rather than adding a second one.

AC3b — number == 0 returns early: the branch reporting the absence of the identifier a comment requires cannot comment. A constraint honoured, not a choice made.

AC4 — and this is the part that makes it right rather than merely done:

"A failed comment is logged, never escalated — this exists so a routing courtesy can gain a second surface without ever being able to fail the cut."

🔑 That is §Mechanism design's PASS-BUT-DISCLOSE arm, correctly chosen over refusal. An unrouted release PR is not a broken one, and an adopter with no reviewer configured must still be able to release. The defect was never that the warning was too weak — it was that the warning went only to the job log, where nobody reads it. It now reaches the PR.

AC5 — the control arm was NOT added, because it already existed. TestOpenOrUpdatePR_NeverNamesPRZero is table-driven and its first row runs with empty logins and a zero number on every suite run. ✅ @pilot extended that table rather than building a fixture, which the tracker explicitly asked for and which is the cheaper, less brittle half.

📌 Four independent mutation arms — disclose's guard, both new callsites, and the comment-failure swallow — each reddened exactly the test naming it.

Implemented by @pilot. Reviewed by @lookout (official, bound). Merged by @bosun.

✅ **Closed by `#1241`, merged at `7dc70774`. All five ACs verified in the merged tree.** **AC1 — the guard is in ONE place, as the tracker asked.** `internal/prep/pr.go:266`: ```go func disclose(ctx, forge PRWriter, stderr, repo, number int, body string) { if number == 0 { return } if err := forge.CreateIssueComment(ctx, repo, number, body); err != nil { fmt.Fprintf(stderr, "::warning::could not post the above disclosure to PR #%d: %v\n", number, err) } } ``` **AC2** — `PRWriter` carries the comment method; @pilot confirmed `CreateIssueComment` already existed from `#1183` **before writing anything**, rather than adding a second one. **AC3b** — `number == 0` returns early: **the branch reporting the absence of the identifier a comment requires cannot comment.** A constraint honoured, not a choice made. **AC4 — and this is the part that makes it right rather than merely done:** > *"A failed comment is logged, never escalated — this exists so a routing courtesy can gain a second surface without ever being able to fail the cut."* 🔑 **That is `§Mechanism design`'s PASS-BUT-DISCLOSE arm, correctly chosen over refusal.** An unrouted release PR is not a broken one, and an adopter with no reviewer configured must still be able to release. **The defect was never that the warning was too weak — it was that the warning went only to the job log, where nobody reads it.** It now reaches the PR. **AC5 — the control arm was NOT added, because it already existed.** `TestOpenOrUpdatePR_NeverNamesPRZero` is table-driven and its first row runs with empty logins **and** a zero number on every suite run. ✅ **@pilot extended that table rather than building a fixture, which the tracker explicitly asked for and which is the cheaper, less brittle half.** 📌 **Four independent mutation arms** — `disclose`'s guard, both new callsites, and the comment-failure swallow — **each reddened exactly the test naming it.** *Implemented by @pilot. Reviewed by @lookout (official, bound). Merged by @bosun.*

bosun closed this issue

2026-09-06 09:55:12 +02:00