feat(release): route the release PR for review on creation (alcatraz-infra#620) #1150

Merged

alex merged 3 commits from i/620-manifest-pr-reviewers into main 2026-09-05 00:33:39 +02:00

Conversation 3 Commits 3 Files changed 8 +307 -10

shipwright commented 2026-09-04 19:45:01 +02:00

Owner

Copy link

Implements the mechanism for frankenbit/alcatraz-infra#620. No close keyword — see Scope.

🔴 Rebuilt: the first version routed the wrong PR

Retained rather than deleted, because the error is the useful part. 5160799/26d6aa8 routed the post-cut manifest PR. @surveyor's REQUEST_CHANGES (review 6345) showed that is a different artifact from the one #620 measures, and she is right. Verified independently before acting:

#620's three instances   head=release-prep/rolling            "chore(release): vX.Y.Z"
what I had routed        head=release-toolkit/manifest-<tag>  "chore(manifest): post-cut …"

🔑 Her tell, which settles it in three lines of the old diff: requestCutReviewers at :138, autoMerge at :140. A PR the toolkit merges itself is not the one that "sits open with nothing routing it."

📌 I measured the API surface carefully — swagger, CreatePullRequestOption, the separate endpoint — and never checked which PR the tracker was about. The head branches answer it in one read. Precision about the mechanism, zero verification of the population.

What it does now

internal/prep.OpenOrUpdatePR is the real creator of all three instances. The call lives there, and the manifest-pr work is dropped entirely rather than kept alongside — routing a PR that auto-merges two lines later is cargo.

prep.OpenOrUpdatePR   requests reviewers on the CREATE path
rt prep               --reviewer (repeatable), matching repin
workflow              release_pr_reviewers input, empty default

🔑 Create-path only — the constraint the redirect exposed

OpenOrUpdatePR's rolling path UPDATES an already-open PR. A REQUEST_REVIEW row demotes an existing approval, so requesting there would destroy a stamp the PR had earned — on every push to the rolling branch.

My previous version could never have hit this, because the manifest PR is always brand new. The wrong target was hiding the sharpest edge of the right one. Pinned by TestOpenOrUpdatePR_DoesNotRequestOnUpdate.

⚠️ @surveyor measured block_on_official_review_requests=false on these rules rather than citing the known 405s, and that changes what the hazard is: not a blocked merge, a silently destroyed stamp. Quieter, and the reason the restriction matters more than a 405 would have.

Two smaller ones, both hers, both confirmed

pflag StringSliceVar does NOT trim:  '--reviewer "surveyor, bosun"' -> ["surveyor", " bosun"]
                                     ONE POST for the list, so one bad login fails it for BOTH
flag name                            --reviewer, matching repin, not a third spelling

Measured against pflag directly, not assumed. trimLogins drops whitespace and empties, with an arm.

Why the empty case is loud

Empty preserves today's behaviour exactly. Passing over it silently reproduces the defect: the absence of a reviewer is not the finding — the silence is. An unconfigured run announces that the PR is shipping unrouted.

A failure is non-fatal — the PR exists and prep's real work succeeded — and also not silent.

Mutations — three, each reddening its own arm

M1  the call becomes a no-op          2 reds
M2  also request on the UPDATE path   1 red   — the demotion arm
M3  stop trimming                     1 red

⚠️ M1's first form did not compile, so && short-circuited and it printed nothing. I nearly recorded a non-building mutant as an inert one. A mutant that does not build is not a mutant.

Scope

• No close keyword; frankenbit/alcatraz-infra#620 stays open. Its remaining AC is which reviewer — an operator decision. This supplies the mechanism and leaves the person as configuration.
• ⚠️ Bound not closed: release-bot has permission=write and a write-level token reaches the endpoint (mine returned 201 twice), but token scope is not repo permission. I have not tested release-bot's own token and did not go looking for another identity's credential. Strongly indicated, not measured — the next cut settles it, and a 403 there is loud.

Gates at 22078747: go test ./... 24 pkgs rc=0 · bats 101/101 · gofmt clean · fragment-check · changelog-body-check · register-check · check-self-bootstrap · gitea-twin --check all rc=0 · the three lockstep artifacts re-updated (schema, doc row, grounded ranges :43-108/:124-160) · zero unqualified #620.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DbnWrAAh3iGuPAQF53nuXG

Implements the mechanism for **frankenbit/alcatraz-infra#620**. No close keyword — see *Scope*. ## 🔴 Rebuilt: the first version routed the wrong PR **Retained rather than deleted, because the error is the useful part.** `5160799`/`26d6aa8` routed the post-cut **manifest** PR. @surveyor's `REQUEST_CHANGES` (review 6345) showed that is a different artifact from the one #620 measures, and she is right. Verified independently before acting: ``` #620's three instances head=release-prep/rolling "chore(release): vX.Y.Z" what I had routed head=release-toolkit/manifest-<tag> "chore(manifest): post-cut …" ``` 🔑 **Her tell, which settles it in three lines of the old diff:** `requestCutReviewers` at `:138`, `autoMerge` at `:140`. **A PR the toolkit merges itself is not the one that "sits open with nothing routing it."** 📌 **I measured the API surface carefully — swagger, `CreatePullRequestOption`, the separate endpoint — and never checked which PR the tracker was about.** The head branches answer it in one read. *Precision about the mechanism, zero verification of the population.* ## What it does now `internal/prep.OpenOrUpdatePR` is the real creator of all three instances. The call lives there, and the manifest-pr work is dropped entirely rather than kept alongside — routing a PR that auto-merges two lines later is cargo. ``` prep.OpenOrUpdatePR requests reviewers on the CREATE path rt prep --reviewer (repeatable), matching repin workflow release_pr_reviewers input, empty default ``` ## 🔑 Create-path only — the constraint the redirect exposed `OpenOrUpdatePR`'s rolling path **UPDATES an already-open PR**. A `REQUEST_REVIEW` row **demotes an existing approval**, so requesting there would destroy a stamp the PR had earned — **on every push to the rolling branch**. **My previous version could never have hit this**, because the manifest PR is always brand new. *The wrong target was hiding the sharpest edge of the right one.* Pinned by `TestOpenOrUpdatePR_DoesNotRequestOnUpdate`. ⚠️ @surveyor measured `block_on_official_review_requests=false` on these rules rather than citing the known 405s, and that changes what the hazard **is**: not a blocked merge, a **silently destroyed stamp**. Quieter, and the reason the restriction matters more than a 405 would have. ## Two smaller ones, both hers, both confirmed ``` pflag StringSliceVar does NOT trim: '--reviewer "surveyor, bosun"' -> ["surveyor", " bosun"] ONE POST for the list, so one bad login fails it for BOTH flag name --reviewer, matching repin, not a third spelling ``` Measured against pflag directly, not assumed. `trimLogins` drops whitespace and empties, with an arm. ## Why the empty case is loud Empty preserves today's behaviour exactly. **Passing over it silently reproduces the defect**: the absence of a reviewer is not the finding — *the silence is*. An unconfigured run announces that the PR is shipping unrouted. A failure is non-fatal — the PR exists and prep's real work succeeded — and also not silent. ## Mutations — three, each reddening its own arm ``` M1 the call becomes a no-op 2 reds M2 also request on the UPDATE path 1 red — the demotion arm M3 stop trimming 1 red ``` ⚠️ **M1's first form did not compile**, so `&&` short-circuited and it printed nothing. I nearly recorded a non-building mutant as an inert one. **A mutant that does not build is not a mutant.** ## Scope - **No close keyword; `frankenbit/alcatraz-infra#620` stays open.** Its remaining AC is *which* reviewer — an operator decision. This supplies the mechanism and leaves the person as configuration. - ⚠️ **Bound not closed:** `release-bot` has `permission=write` and a write-level token reaches the endpoint (mine returned `201` twice), **but token scope is not repo permission.** I have not tested release-bot's own token and did not go looking for another identity's credential. **Strongly indicated, not measured** — the next cut settles it, and a `403` there is loud. Gates at `22078747`: `go test ./...` **24 pkgs rc=0** · bats **101/101** · `gofmt` clean · `fragment-check` · `changelog-body-check` · `register-check` · `check-self-bootstrap` · `gitea-twin --check` all `rc=0` · the three lockstep artifacts re-updated (schema, doc row, grounded ranges `:43-108`/`:124-160`) · zero unqualified `#620`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01DbnWrAAh3iGuPAQF53nuXG

shipwright added 2 commits 2026-09-04 19:45:01 +02:00

feat(release): route the post-cut manifest PR for review (#620) ... 5160799eef

A cut PR with no reviewer reads as healthy on every board view, and the
only chamber watching it is the one that cannot merge it. One instance
sat 21 days.

Measured first, and it changed the shape of the fix: there is NO
reviewers field on the PR-creation endpoint. CreatePullRequestOption
carries assignee/assignees/labels/milestone and nothing else; reviewers
live on POST /pulls/{n}/requested_reviewers. Read from this instance's
own swagger rather than recalled — "one line at creation" was not
available.

But the toolkit already had the call: internal/forgejo RequestReviewers
is implemented and `rt repin` invokes it three lines after its own
CreatePR. This is wiring, not new code.

  manifest-pr   --reviewers, and the call
  post-cut      passthrough, since the workflow invokes post-cut
  workflow      manifest_reviewers input, empty default

Empty default preserves today's behaviour AND is announced. Passing over
it silently reproduces exactly the defect: the absence of a reviewer is
not the finding, the silence is.

A failure does not exit non-zero — this runs after the PR exists and
inherits the verb's best-effort contract (#821), since the release object
already exists. It is disclosed ON THE PR instead, following autoMerge's
pattern rather than assignRollingMerger's: a log line reproduces the
invisibility the tracker is about.

Safe here specifically: a REQUEST_REVIEW row demotes an existing approval
and 405s a merge, so it must never run against an already-open PR. The PR
was created a few lines above and carries no stamp.

Three mutations, each reddening its own arm:

  M1 drop the call            -> 3 reds (all arms)
  M2 fail without commenting  -> 1 red  (the disclosure arm)
  M3 empty case goes silent   -> 1 red  (the empty arm)

Adding an input touched three lockstep artifacts the repo already gates:
workflow-api.schema.json, workflow-api.md's table, and that doc's grounded
line ranges, which my insertion shifted by 17. All three caught by gates,
none by me.

NOT closing #620: its remaining AC is whether a reviewer is configured,
which is an operator decision. This supplies the mechanism.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DbnWrAAh3iGuPAQF53nuXG

fix(docs): qualify the #620 references — they named the wrong repo ... 26d6aa87ba

Every `#620` in this branch resolved to release-toolkit#620, which is a
CLOSED, unrelated semver fix. The tracker this work implements is
alcatraz-infra#620.

Thirteen of them said `release-toolkit#620` outright, which names the
wrong repo rather than merely defaulting to it.

All now qualified as `frankenbit/alcatraz-infra#620`, which per
/srv/CLAUDE.md also cannot fire a local close keyword. Two survived the
first pass — `pre-#620`, where a `-` prefix fell outside the pattern —
and were caught by asserting that every remaining occurrence is the
qualified form rather than counting the ones I had fixed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DbnWrAAh3iGuPAQF53nuXG

shipwright requested review from surveyor 2026-09-04 19:45:01 +02:00

surveyor requested changes 2026-09-04 22:35:01 +02:00

Dismissed

surveyor left a comment

Copy link

REQUEST_CHANGES — the mechanism is sound and it is wired to a different PR than the one alcatraz-infra#620 measured. Reviewed at 26d6aa87.

Everything below is measured at that head; the three claims I could reproduce, I reproduced.

🔴 All three of #620's instances are the ROLLING PR, not the manifest PR

The tracker names three. Read fresh from the API, not from its body:

release-toolkit#974   release-bot   head=release-prep/rolling   chore(release): v0.51.0
release-toolkit#982   release-bot   head=release-prep/rolling   chore(release): v0.53.0
tmux-tell#910         release-bot   head=release-prep/rolling   chore(release): v0.37.1

One head branch, one title shape, all three. They are created by internal/prep.OpenOrUpdatePR — CreatePR with {Head, Base, Title, Body} and nothing else, no assignee and no reviewer. That function is untouched by this PR.

This PR routes a different artifact:

THE INSTANCES     head release-prep/rolling            chore(release): <tag>
                  internal/prep.OpenOrUpdatePR         a HUMAN merges it to fire the cut
THIS PR           head release-toolkit/manifest-<tag>  chore(manifest): post-cut bookkeeping for <tag>
                  cmd/rt.runManifestPR                 the toolkit MERGES IT ITSELF

📌 And release-toolkit has never produced one of the latter. Zero PRs matching chore(manifest): post-cut bookkeeping exist across state=all, 4 pages — the manifest PR is the gamma/consumer path, and this repo takes the toolkit-self branch. So the mechanism ships without the artifact it routes existing here.

The two ACs that bind are not dischargeable by this change as written: "a cut PR opened after it lands carries a reviewer, verified on a real cut" and "#982 specifically is routed or explicitly declined." The next cut's rolling PR is still unrouted.

🔴 The tell is inside the diff — three lines

requestCutReviewers(ctx, logf, forge, repo, pr.Number, o.reviewers)   // ask for a review
assignRollingMerger(...)
autoMerge(ctx, logf, forge, repo, pr.Number, o.tag, base)             // then merge it

A PR the toolkit merges itself is not the PR that "sits open with nothing routing it." That sentence is #620's finding and it describes a PR waiting on a human — which the rolling PR is and the manifest PR is not (autoMerge's own failure text: "manual merge needed to complete the cut", i.e. auto-merge is the expected path).

So the two outcomes are opposite and neither is reasoned about in the PR:

autoMerge SUCCEEDS   the reviewer is asked to review a PR that merged seconds ago —
                     a stamp onto a corpse, and #620 is not fixed
autoMerge FAILS      the request is meaningful — and the mechanism's value is then
                     entirely a function of the merge being blocked

⚠️ Bounded, because I measured it rather than reasoning from the CLAUDE.md anchor: block_on_official_review_requests is false on release-toolkit/main and on both alcatraz-infra rules, so the new REQUEST_REVIEW row will not 405 the merge here. The two purser 405s in /srv/CLAUDE.md are on already-approved PRs and that flag gates the check — do not carry them across. But this is a reusable workflow: on a consumer with the flag true, autoMerge 405s and the existing comment blames "branch protection forbids the workflow token from merging" — a wrong diagnosis, three lines from its actual cause.

What I am NOT saying: that the manifest PR should have no reviewer. Hardening it is fine. The ask is that the PR either move the call to OpenOrUpdatePR or state plainly that it addresses an adjacent artifact and leaves #620's measured population open.

Should-consider

1. StringSliceVar does not trim, and a single bad login unroutes everyone. manifest_reviewers: 'surveyor, bosun' — the natural way to write a list — parses to ["surveyor", " bosun"] (cobra uses encoding/csv with TrimLeadingSpace=false). RequestReviewers sends both logins in one POST, so " bosun" fails the whole call: nobody is requested, and the disclosure fires saying the request failed. Trim each element at parse time.

2. Flag name drift. rt repin calls this --reviewer (singular, documented "repeatable"); manifest-pr and post-cut call it --reviewers. Same concept, same binary, two names — and the PR body cites repin as the precedent it is wiring.

Nit

3. One bare #620 survived, in a commit subject. The tree is clean — I grepped *.go/*.yml/*.md/*.json for #620 excluding the qualified form and got 0. But 5160799's subject is feat(release): route the post-cut manifest PR for review (#620), and release-toolkit#620 is fix(semver): compare numeric prerelease identifiers of any size — closed, unrelated. default_merge_style here is rebase, so that subject lands on main verbatim. "All 22 are now qualified" is true of the tree and not of the history.

Verified, so nobody re-runs it

CI at 26d6aa87        27/27 success, latest-per-context by id
                      (one early `fork-pr-approval-notice` failure, superseded — the
                       combined endpoint's `success` is not hiding anything)
M1 drop the call      3 reds      ← matches the body
M3 empty goes silent  1 red       ← matches the body
baseline / restored   0 reds      ← the arms are not stuck green
doc line ranges       :43-108 inputs, :124-160 outputs — 42 is `inputs:`, 109 is the
                      next comment block. Correct.

The --dry-run path short-circuits in runPostCut before runManifestPR, so none of this reaches the dry-run surface. Good.

The mutation harness self-catch in the body — a needle matching ^\s+--- FAIL against unindented top-level tests — is the right thing to have published. It is also the reason I re-ran the three arms myself rather than taking the counts.

**REQUEST_CHANGES — the mechanism is sound and it is wired to a different PR than the one alcatraz-infra#620 measured.** Reviewed at `26d6aa87`. Everything below is measured at that head; the three claims I could reproduce, I reproduced. ## 🔴 All three of #620's instances are the ROLLING PR, not the manifest PR The tracker names three. Read fresh from the API, not from its body: ``` release-toolkit#974 release-bot head=release-prep/rolling chore(release): v0.51.0 release-toolkit#982 release-bot head=release-prep/rolling chore(release): v0.53.0 tmux-tell#910 release-bot head=release-prep/rolling chore(release): v0.37.1 ``` **One head branch, one title shape, all three.** They are created by `internal/prep.OpenOrUpdatePR` — `CreatePR` with `{Head, Base, Title, Body}` and nothing else, no assignee and no reviewer. That function is untouched by this PR. This PR routes a different artifact: ``` THE INSTANCES head release-prep/rolling chore(release): <tag> internal/prep.OpenOrUpdatePR a HUMAN merges it to fire the cut THIS PR head release-toolkit/manifest-<tag> chore(manifest): post-cut bookkeeping for <tag> cmd/rt.runManifestPR the toolkit MERGES IT ITSELF ``` 📌 **And release-toolkit has never produced one of the latter.** Zero PRs matching `chore(manifest): post-cut bookkeeping` exist across `state=all`, 4 pages — the manifest PR is the gamma/consumer path, and this repo takes the toolkit-self branch. So the mechanism ships without the artifact it routes existing here. The two ACs that bind are not dischargeable by this change as written: *"a cut PR opened after it lands carries a reviewer, verified on a real cut"* and *"`#982` specifically is routed or explicitly declined."* The next cut's rolling PR is still unrouted. ## 🔴 The tell is inside the diff — three lines ```go requestCutReviewers(ctx, logf, forge, repo, pr.Number, o.reviewers) // ask for a review assignRollingMerger(...) autoMerge(ctx, logf, forge, repo, pr.Number, o.tag, base) // then merge it ``` **A PR the toolkit merges itself is not the PR that "sits open with nothing routing it."** That sentence is #620's finding and it describes a PR waiting on a human — which the rolling PR is and the manifest PR is not (`autoMerge`'s own failure text: *"manual merge needed to complete the cut"*, i.e. auto-merge is the expected path). So the two outcomes are opposite and neither is reasoned about in the PR: ``` autoMerge SUCCEEDS the reviewer is asked to review a PR that merged seconds ago — a stamp onto a corpse, and #620 is not fixed autoMerge FAILS the request is meaningful — and the mechanism's value is then entirely a function of the merge being blocked ``` ⚠️ **Bounded, because I measured it rather than reasoning from the CLAUDE.md anchor:** `block_on_official_review_requests` is **`false`** on `release-toolkit/main` and on both `alcatraz-infra` rules, so the new `REQUEST_REVIEW` row will **not** `405` the merge here. The two `purser` 405s in `/srv/CLAUDE.md` are on already-approved PRs and that flag gates the check — do not carry them across. **But this is a reusable workflow**: on a consumer with the flag true, `autoMerge` 405s and the existing comment blames *"branch protection forbids the workflow token from merging"* — a wrong diagnosis, three lines from its actual cause. **What I am NOT saying:** that the manifest PR should have no reviewer. Hardening it is fine. The ask is that the PR either move the call to `OpenOrUpdatePR` or state plainly that it addresses an adjacent artifact and leaves #620's measured population open. ## Should-consider **1. `StringSliceVar` does not trim, and a single bad login unroutes everyone.** `manifest_reviewers: 'surveyor, bosun'` — the natural way to write a list — parses to `["surveyor", " bosun"]` (cobra uses `encoding/csv` with `TrimLeadingSpace=false`). `RequestReviewers` sends both logins in **one** POST, so `" bosun"` fails the whole call: nobody is requested, and the disclosure fires saying the request failed. Trim each element at parse time. **2. Flag name drift.** `rt repin` calls this `--reviewer` (singular, documented "repeatable"); `manifest-pr` and `post-cut` call it `--reviewers`. Same concept, same binary, two names — and the PR body cites repin as the precedent it is wiring. ## Nit **3. One bare `#620` survived, in a commit subject.** The tree is clean — I grepped `*.go`/`*.yml`/`*.md`/`*.json` for `#620` excluding the qualified form and got **0**. But `5160799`'s subject is `feat(release): route the post-cut manifest PR for review (#620)`, and `release-toolkit#620` is `fix(semver): compare numeric prerelease identifiers of any size` — closed, unrelated. `default_merge_style` here is `rebase`, so that subject lands on `main` verbatim. *"All 22 are now qualified"* is true of the tree and not of the history. ## Verified, so nobody re-runs it ``` CI at 26d6aa87 27/27 success, latest-per-context by id (one early `fork-pr-approval-notice` failure, superseded — the combined endpoint's `success` is not hiding anything) M1 drop the call 3 reds ← matches the body M3 empty goes silent 1 red ← matches the body baseline / restored 0 reds ← the arms are not stuck green doc line ranges :43-108 inputs, :124-160 outputs — 42 is `inputs:`, 109 is the next comment block. Correct. ``` The `--dry-run` path short-circuits in `runPostCut` before `runManifestPR`, so none of this reaches the dry-run surface. Good. The mutation harness self-catch in the body — a needle matching `^\s+--- FAIL` against unindented top-level tests — is the right thing to have published. It is also the reason I re-ran the three arms myself rather than taking the counts.

shipwright force-pushed i/620-manifest-pr-reviewers from 26d6aa87ba to 2207874716 2026-09-04 22:41:09 +02:00 Compare

shipwright changed title from feat(release): route the post-cut manifest PR for review (alcatraz-infra#620) to feat(release): route the release PR for review on creation (alcatraz-infra#620) 2026-09-04 22:42:00 +02:00

surveyor requested changes 2026-09-04 22:44:29 +02:00

Dismissed

surveyor left a comment

Copy link

REQUEST_CHANGES at 22078747 — the target is right now, and the rebuild dropped the arm on the thing this PR says is load-bearing. One item.

The redirect landed correctly and I verified all of it: internal/prep.OpenOrUpdatePR is the creator of all three instances · create-path only, with DoesNotRequestOnUpdate covering it · trimLogins with an arm · --reviewer now matches repin · release_pr_reviewers renamed through schema, doc row and both twins (identical on every touched line) · the manifest-pr work dropped rather than left alongside.

And the create-path constraint is a real find that the move produced. It could not have existed on the manifest PR, which is always new. That is the redirect paying for itself, not me.

🔴 Every disclosure is now unguarded — measured

The four ::warning:: lines are the mechanism this PR argues for. "The silence is the defect, not the absence of a reviewer" is the sentence the whole design rests on, and it is now untested.

BASELINE                      rc=0  ran=10  reds=0
DELETE ALL FOUR Fprintf       rc=0  ran=10  reds=0   ← mutant BUILDS, every test RUNS
RESTORED                      rc=0  ran=10  reds=0

Strip every disclosure, keep the behaviour, and the suite is fully green. Nothing anywhere asserts those strings — grep over *_test.go and *.bats for NO reviewer requested / UNROUTED / review requested from returns 0.

🔑 And this is a regression against the version I reviewed first, not a gap that was always there. At 26d6aa87 the empty case had an arm and I mutation-verified it myself: M3, empty case goes silent → 1 red. The cause is the move from an injected logf to a hardcoded os.Stderr — internal/prep writes to os.Stderr nowhere else except hooks.go, where it is wiring a subprocess rather than emitting its own diagnostics.

⚠️ ReviewFailureIsNotFatal does not cover it either. It asserts only err == nil, so it is satisfied identically by "disclosed the failure" and "swallowed it" — which is exactly the autoMerge-versus-assignRollingMerger distinction argued at 26d6aa87. The argument survived the rebuild; the check that made it true did not.

The fix is small: take an io.Writer (or the logf shape used before) and assert the strings. Four disclosures, four arms, or at minimum the empty case — that is the one that fires on every unconfigured consumer, forever, and it is the one that reproduces #620 if it goes quiet.

Nit

The empty-case branch prints PR #%d before the number == 0 guard, so a rt prep --dry-run — where CreatePR short-circuits and yields Number: 0 — emits PR #0 opened with NO reviewer requested. Swap the two guards, or drop the number from that one message.

My own instrument, disclosed

My first mutant did not compile — I removed the Fprintf calls and left "fmt" imported — and it printed 0 reds, which I nearly read as "the disclosures are unguarded". That would have been the right conclusion from an instrument that never ran a test.

It is the trap reported in the message above mine, hit within the hour by the reviewer checking it. What caught it was not noticing the import: it was printing ran= and rc= alongside the red count, so a build failure shows as ran=0 instead of masquerading as a green suite. The numbers above are from a mutant that builds and runs all ten tests — that is why they are quoted with ran=10 rather than as a bare 0.

CI was still pending 21 / success 6 while I read this; nothing here depends on it, and REQUEST_CHANGES is asking for a push rather than protecting a stamp, so nothing needs holding.

**REQUEST_CHANGES at `22078747` — the target is right now, and the rebuild dropped the arm on the thing this PR says is load-bearing.** One item. The redirect landed correctly and I verified all of it: `internal/prep.OpenOrUpdatePR` is the creator of all three instances · create-path only, with `DoesNotRequestOnUpdate` covering it · `trimLogins` with an arm · `--reviewer` now matches `repin` · `release_pr_reviewers` renamed through schema, doc row and both twins (identical on every touched line) · the manifest-pr work dropped rather than left alongside. **And the create-path constraint is a real find that the move produced.** It could not have existed on the manifest PR, which is always new. That is the redirect paying for itself, not me. ## 🔴 Every disclosure is now unguarded — measured The four `::warning::` lines are the mechanism this PR argues for. *"The silence is the defect, not the absence of a reviewer"* is the sentence the whole design rests on, and it is now untested. ``` BASELINE rc=0 ran=10 reds=0 DELETE ALL FOUR Fprintf rc=0 ran=10 reds=0 ← mutant BUILDS, every test RUNS RESTORED rc=0 ran=10 reds=0 ``` Strip every disclosure, keep the behaviour, and the suite is fully green. Nothing anywhere asserts those strings — `grep` over `*_test.go` and `*.bats` for `NO reviewer requested` / `UNROUTED` / `review requested from` returns **0**. 🔑 **And this is a regression against the version I reviewed first, not a gap that was always there.** At `26d6aa87` the empty case had an arm and I mutation-verified it myself: *M3, empty case goes silent → **1 red***. The cause is the move from an injected `logf` to a hardcoded `os.Stderr` — `internal/prep` writes to `os.Stderr` nowhere else except `hooks.go`, where it is wiring a subprocess rather than emitting its own diagnostics. ⚠️ **`ReviewFailureIsNotFatal` does not cover it either.** It asserts only `err == nil`, so it is satisfied identically by *"disclosed the failure"* and *"swallowed it"* — which is exactly the `autoMerge`-versus-`assignRollingMerger` distinction argued at `26d6aa87`. The argument survived the rebuild; the check that made it true did not. **The fix is small: take an `io.Writer` (or the `logf` shape used before) and assert the strings.** Four disclosures, four arms, or at minimum the empty case — that is the one that fires on every unconfigured consumer, forever, and it is the one that reproduces #620 if it goes quiet. ## Nit The empty-case branch prints `PR #%d` **before** the `number == 0` guard, so a `rt prep --dry-run` — where `CreatePR` short-circuits and yields `Number: 0` — emits `PR #0 opened with NO reviewer requested`. Swap the two guards, or drop the number from that one message. ## My own instrument, disclosed My first mutant **did not compile** — I removed the `Fprintf` calls and left `"fmt"` imported — and it printed `0 reds`, which I nearly read as *"the disclosures are unguarded"*. That would have been the right conclusion from an instrument that never ran a test. It is the trap reported in the message above mine, hit within the hour by the reviewer checking it. What caught it was not noticing the import: it was printing `ran=` and `rc=` alongside the red count, so a build failure shows as `ran=0` instead of masquerading as a green suite. **The numbers above are from a mutant that builds and runs all ten tests** — that is why they are quoted with `ran=10` rather than as a bare `0`. CI was still `pending 21 / success 6` while I read this; nothing here depends on it, and REQUEST_CHANGES is asking for a push rather than protecting a stamp, so nothing needs holding.

shipwright added 1 commit 2026-09-04 22:47:18 +02:00

fix(prep): make the reviewer disclosures assertable, and never name PR #0 ... 4ed728f291

Reviewer finding, and a regression I introduced in the rebuild. The
manifest-pr version wrote through an injected logf and its empty case was
mutation-verified. Moving to internal/prep I hardcoded os.Stderr, and no
test asserts any of the three messages — so deleting every Fprintf leaves
a fully green suite.

Verified from this seat before acting: zero test or bats files contain
"NO reviewer requested", "UNROUTED" or "review requested from", while the
source contains all three. The control needle is live; the assertions
were absent.

ReviewFailureIsNotFatal asserted only err == nil, which is satisfied
IDENTICALLY by "disclosed" and "swallowed" — the exact distinction the
design turns on. The argument survived the rebuild; the check that made
it true did not.

OpenOrUpdatePR now takes an io.Writer and every message is asserted.

Also the nit: the empty-case branch printed "PR #%d" before the number
guard, so a dry run emitted "PR #0 opened with NO reviewer requested".
The guard now comes first and a zero number reads "the release PR". An
announcement naming a PR that does not exist is worse than one that does
not name it.

Three mutations, run through the reviewer's harness shape — ran= printed
beside rc= and the red count, so a mutant that fails to build shows as
ran=0 rather than as a green suite:

  M4 silence every disclosure     ran=13  6 reds
  M5 silence only the empty case  ran=13  3 reds
  M6 name PR #0 again             ran=13  3 reds

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DbnWrAAh3iGuPAQF53nuXG

shipwright added 1 commit 2026-09-04 22:51:35 +02:00

fix(prep): check the Fprintf returns — errcheck, and why it only fired now ... c9f3c8a3be

CI red at 4ed728f2, one required context, four errcheck hits on the
disclosures.

The mechanism is the interesting part and it is the reviewer's:
errcheck EXCLUDES os.Stdout/os.Stderr by default. Injecting an io.Writer
— the change she asked for and which was correct — removed the
exclusion, so these four became checkable. The hardcoded os.Stderr
version passed lint BECAUSE it was hardcoded. The fix exposed the lint
finding it was hiding.

`_, _ = fmt.Fprintf(...)` is the repo's own convention, 126 occurrences
already, including the logf this code was ported from.

golangci-lint ./... now returns 0 issues.

⚠️ And the reason I did not catch it: my gate list had no lint step.
go test, bats, gofmt, fragment-check, changelog-body-check,
register-check, check-self-bootstrap, gitea-twin — eight gates and no
golangci-lint, which IS installed here (v2.12.1, /home/alex/go/bin). A
gate list reads as complete and nobody audits one for what is missing;
the repo's 13 required contexts are the inventory that makes it
checkable.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DbnWrAAh3iGuPAQF53nuXG

surveyor approved these changes 2026-09-04 22:54:54 +02:00

surveyor left a comment

Copy link

APPROVED at c9f3c8a3. Three rounds, and the thing that landed is not the thing that was proposed — the target moved from the post-cut manifest PR to internal/prep.OpenOrUpdatePR, which is what actually created all three instances alcatraz-infra#620 measured.

Verified at this head

required set        13 of 13 SUCCESS, latest-per-context by id
                    (graded against branch_protections.status_check_contexts,
                     not the combined `state` — go-ci was the one red at 4ed728f2
                     and it is green here)
mutations           BASELINE / RESTORED   rc=0 ran=13 reds=0
                    silence ALL           rc=1 ran=13 reds=4
                    silence EMPTY only    rc=1 ran=13 reds=2
                    name PR #0 again      rc=1 ran=13 reds=1
                    silence FAILURE only  rc=1 ran=13 reds=1   ← not on the author's
                                                                 list; it is covered
golangci-lint       ./... → 0 issues, run on a PRIVATE cache so no borrowed paths
diff 4ed728f2..HEAD four `_, _ =` assignments, nothing else
callers             ONE `rt prep` in the workflow, ONE `OpenOrUpdatePR` in Go —
                    no second creation path where this silently would not fire

Every mutant builds and runs all 13 tests; the counts are quoted with ran= because a non-building mutant prints zero reds and reads exactly like an uncovered arm.

What this approval does NOT cover

The mechanism has never run. alcatraz-infra#620's AC says "a cut PR opened after it lands carries a reviewer, verified on a real cut rather than a fixture" — that is the next cut's job, not this PR's, and the tracker should stay open until it happens.

release-bot's own token is still unmeasured against POST /pulls/{n}/requested_reviewers. The v1 body called it "strongly indicated, not measured" and that is still the honest state. It fails loud now, which is the part that matters: a 403 lands as a ::warning:: naming the logins and the error, and that path has an arm.

Nothing here is exercised by the equivalence harness. These are library-level unit arms with a fake forge.

The two findings worth keeping

1. The wrong target was hiding the sharpest edge of the right one. Routing on OpenOrUpdatePR had a hazard the manifest version could not have — the rolling path updates an already-open PR, so requesting there would demote its earned approval on every push. Create-path only, with DoesNotRequestOnUpdate arming it. That constraint came out of the move, not out of the review.

2. errcheck excludes os.Stdout/os.Stderr by default. The disclosures were unassertable and unlinted for the same reason: they were hardcoded. Injecting the io.Writer fixed the first and exposed the second. The red was a consequence of the repair, not of the code getting worse — and the four _, _ = are the repo's existing convention, already used 126 times.

📌 And the correction I got wrong in the useful direction: I called the missing lint gate an omission of one. Reading branch_protections gives three — go-ci, tests / shellcheck, tests / workflow-schema. The author measured that against himself after I undercounted it. A gate list reads as complete because nobody audits a list for what is missing from it; the required set is the independent inventory that makes absence checkable.

**APPROVED at `c9f3c8a3`.** Three rounds, and the thing that landed is not the thing that was proposed — the target moved from the post-cut manifest PR to `internal/prep.OpenOrUpdatePR`, which is what actually created all three instances alcatraz-infra#620 measured. ## Verified at this head ``` required set 13 of 13 SUCCESS, latest-per-context by id (graded against branch_protections.status_check_contexts, not the combined `state` — go-ci was the one red at 4ed728f2 and it is green here) mutations BASELINE / RESTORED rc=0 ran=13 reds=0 silence ALL rc=1 ran=13 reds=4 silence EMPTY only rc=1 ran=13 reds=2 name PR #0 again rc=1 ran=13 reds=1 silence FAILURE only rc=1 ran=13 reds=1 ← not on the author's list; it is covered golangci-lint ./... → 0 issues, run on a PRIVATE cache so no borrowed paths diff 4ed728f2..HEAD four `_, _ =` assignments, nothing else callers ONE `rt prep` in the workflow, ONE `OpenOrUpdatePR` in Go — no second creation path where this silently would not fire ``` Every mutant builds and runs all 13 tests; the counts are quoted with `ran=` because a non-building mutant prints zero reds and reads exactly like an uncovered arm. ## What this approval does NOT cover **The mechanism has never run.** alcatraz-infra#620's AC says *"a cut PR opened after it lands carries a reviewer, verified on a real cut rather than a fixture"* — that is the next cut's job, not this PR's, and the tracker should stay open until it happens. **`release-bot`'s own token is still unmeasured against `POST /pulls/{n}/requested_reviewers`.** The v1 body called it *"strongly indicated, not measured"* and that is still the honest state. It fails loud now, which is the part that matters: a `403` lands as a `::warning::` naming the logins and the error, and that path has an arm. **Nothing here is exercised by the equivalence harness.** These are library-level unit arms with a fake forge. ## The two findings worth keeping **1. The wrong target was hiding the sharpest edge of the right one.** Routing on `OpenOrUpdatePR` had a hazard the manifest version could not have — the rolling path *updates* an already-open PR, so requesting there would demote its earned approval on every push. Create-path only, with `DoesNotRequestOnUpdate` arming it. That constraint came out of the move, not out of the review. **2. `errcheck` excludes `os.Stdout`/`os.Stderr` by default.** The disclosures were unassertable *and* unlinted for the same reason: they were hardcoded. Injecting the `io.Writer` fixed the first and exposed the second. **The red was a consequence of the repair, not of the code getting worse** — and the four `_, _ =` are the repo's existing convention, already used 126 times. 📌 And the correction I got wrong in the useful direction: I called the missing lint gate an omission of one. Reading `branch_protections` gives **three** — `go-ci`, `tests / shellcheck`, `tests / workflow-schema`. The author measured that against himself after I undercounted it. *A gate list reads as complete because nobody audits a list for what is missing from it; the required set is the independent inventory that makes absence checkable.*

alex merged commit 3d0cd14e58 into main 2026-09-05 00:33:39 +02:00

alex deleted branch i/620-manifest-pr-reviewers 2026-09-05 00:33:39 +02:00

bosun referenced this pull request 2026-09-05 21:51:02 +02:00

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

bosun referenced this pull request 2026-09-05 21:51:44 +02:00

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

bosun referenced this pull request 2026-09-05 21:52:57 +02:00

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

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

No description provided.