fix(cli): adopter-preflight exits 2 when it graded nothing (#1357) #1365

Merged

bosun merged 1 commit from i/1357-preflight-read-nothing into main 2026-09-06 18:22:00 +02:00

Conversation 2 Commits 1 Files changed 6 +231 -5

shipwright commented 2026-09-06 18:10:13 +02:00

Owner

Copy link

rt adopter-preflight exited 0 having read zero forge-side facts, so an adopter who measured nothing was told they were ready. It now exits 2.

Refs #1357. The tracker stays open until this merges and is then closed by hand.

The distinction that carries it

ALL forge scopes unreadable         a CREDENTIAL failure. This run graded NOTHING.
one scope unread beside readable    a PERMISSION BOUNDARY. A complete run.

🔴 The obvious fix — map cannot-grade to rc=2 — is wrong, and it is wrong in a way that only shows up on adopters we do not have. The instance-runners endpoint is site-admin-only, so an ordinary repository owner ALWAYS has one unread scope. A wholesale mapping reddens every legitimate adopter permanently.

That is why the control is a separate arm rather than a second assertion: TestAdopterPreflight_OneUnreadScopeBesideReadableOnesStillGrades fails if the fix is ever generalised into the defect it was written to avoid. I wrote it first, before the fix, so that I could not write a fix that passed only AC1.

📌 And the predicate reads the SCOPES, never the findings. A run where every scope answered and every answer was empty is a complete measurement of an empty forge — still a definitive FAIL, with its own arm pinning it.

Verified against the consumer that found it

FrankenBit/rt-gitea-adopter-probe, no token   rc=2  adopter-preflight=cannot-grade
our forge, valid token, instance unread       rc=0  adopter-preflight=warn   <- the control

Both are real runs against real forges, not fixtures. The first is the exact invocation that produced the finding.

Five mutations, each applied, each reddening its own arms

mutation	arms reddened
P1 anyScopeReadable always true	NothingReadableIsCannotGrade, …ExitsCannotGrade
P2 anyScopeReadable always false	6 arms, including the boundary control
P3 invert the err == nil test	NothingReadableIsCannotGrade, …ExitsCannotGrade
P4 let warns win before the check	ReportsEveryMissing…, ReadsEveryScope
P5 command stops mapping to rc=2	NothingReadableExitsCannotGrade
control	green, 17 arms

⚠️ P4 is coarser than the ordering property it targets — it short-circuits fails as well as the cannot-grade branch — so I am recording it as reddening rather than as a clean ordering control. An earlier attempt at a precise ordering mutation did not compile and was discarded rather than scored.

🔑 P5 exists because the grader and the exit code are DIFFERENT SURFACES. Grading correctly and exiting 0 anyway would satisfy every other arm in this file — which is precisely the defect being fixed, so it needed its own end-to-end arm driving the cobra command against a 401 server.

AC coverage

• AC1 rc=2 when nothing was read, rc=0 when something was — the two real runs above, plus TestAdopterPreflightCmd_NothingReadableExitsCannotGrade.
• AC2 the summary token distinguishes — adopter-preflight=cannot-grade vs =warn.
• AC3 an arm covers the all-unreadable case and reddens on its own mutation — P1, P3, P5.
• AC4 the single-unread-scope case still exits 0 — its own arm, and the live control run.

The enum

gates.PreflightDecision gains a case rather than changing one. PreflightPushWhitelist never returns it and is untouched; there is no exhaustive switch on the type anywhere in the tree (checked, not assumed).

The blast-radius sweep, done before a reviewer had to ask

The exit contract was stated in three places and all three now say the same thing: the verb's own header comment, docs/integration.md's adopter-facing block, and cli-surface.md's row.

📌 That sweep is #1355's lesson applied one PR later. There I corrected a false sentence where I happened to be reading and left the copy I had written first; @sentry found it. The blast radius of a changed contract is every place that stated the old one, and the surviving copy is systematically the earlier one — because the later copy is what drew the review.

What this does NOT do

• It does not change what the tooling line measures. It still describes the process it runs in, and still says so. Running the preflight inside the job image remains the only way to make that line authoritative — #1259's AC3, still blocked on a gitea.com credential.
• It does not touch preflight-push-whitelist's mapping, which the old comment cited as precedent. If that verb has the same hole it is a separate finding and I have not measured it.
• It does not distinguish why every scope refused — a dead credential, a wrong base URL and a network outage all land on rc=2. That is correct for this AC: all three mean you have not been graded.

Verification

fragment-check rc=0 · changelog-body-check rc=0 · register-check rc=0 (file scan and commit-subject scan) · go build ./... rc=0 · go test ./... rc=0 · bats tests/workflows.bats 121 ok / 0 not ok.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DbnWrAAh3iGuPAQF53nuXG

`rt adopter-preflight` exited 0 having read zero forge-side facts, so an adopter who measured nothing was told they were ready. It now exits 2. `Refs #1357.` **The tracker stays open until this merges and is then closed by hand.** ## The distinction that carries it ``` ALL forge scopes unreadable a CREDENTIAL failure. This run graded NOTHING. one scope unread beside readable a PERMISSION BOUNDARY. A complete run. ``` 🔴 **The obvious fix — map cannot-grade to `rc=2` — is wrong, and it is wrong in a way that only shows up on adopters we do not have.** The instance-runners endpoint is site-admin-only, so **an ordinary repository owner ALWAYS has one unread scope.** A wholesale mapping reddens every legitimate adopter permanently. **That is why the control is a separate arm rather than a second assertion**: `TestAdopterPreflight_OneUnreadScopeBesideReadableOnesStillGrades` fails if the fix is ever generalised into the defect it was written to avoid. **I wrote it first, before the fix, so that I could not write a fix that passed only AC1.** 📌 **And the predicate reads the SCOPES, never the findings.** A run where every scope answered and every answer was empty is a *complete measurement of an empty forge* — still a definitive FAIL, with its own arm pinning it. ## Verified against the consumer that found it ``` FrankenBit/rt-gitea-adopter-probe, no token rc=2 adopter-preflight=cannot-grade our forge, valid token, instance unread rc=0 adopter-preflight=warn <- the control ``` **Both are real runs against real forges, not fixtures.** The first is the exact invocation that produced the finding. ## Five mutations, each applied, each reddening its own arms | mutation | arms reddened | |---|---| | P1 `anyScopeReadable` always true | `NothingReadableIsCannotGrade`, `…ExitsCannotGrade` | | P2 `anyScopeReadable` always false | 6 arms, including the boundary control | | P3 invert the `err == nil` test | `NothingReadableIsCannotGrade`, `…ExitsCannotGrade` | | P4 let `warns` win before the check | `ReportsEveryMissing…`, `ReadsEveryScope` | | P5 command stops mapping to `rc=2` | `NothingReadableExitsCannotGrade` | | control | green, 17 arms | ⚠️ **P4 is coarser than the ordering property it targets** — it short-circuits `fails` as well as the cannot-grade branch — **so I am recording it as reddening rather than as a clean ordering control.** An earlier attempt at a precise ordering mutation did not compile and was discarded rather than scored. 🔑 **P5 exists because the grader and the exit code are DIFFERENT SURFACES.** Grading correctly and exiting 0 anyway would satisfy every other arm in this file — which is precisely the defect being fixed, so it needed its own end-to-end arm driving the cobra command against a 401 server. ## AC coverage - **AC1** `rc=2` when nothing was read, `rc=0` when something was — the two real runs above, plus `TestAdopterPreflightCmd_NothingReadableExitsCannotGrade`. - **AC2** the summary token distinguishes — `adopter-preflight=cannot-grade` vs `=warn`. - **AC3** an arm covers the all-unreadable case and reddens on its own mutation — P1, P3, P5. - **AC4** the single-unread-scope case still exits 0 — its own arm, and the live control run. ## The enum `gates.PreflightDecision` **gains a case rather than changing one.** `PreflightPushWhitelist` never returns it and is untouched; there is no exhaustive switch on the type anywhere in the tree (checked, not assumed). ## The blast-radius sweep, done before a reviewer had to ask **The exit contract was stated in three places and all three now say the same thing:** the verb's own header comment, `docs/integration.md`'s adopter-facing block, and `cli-surface.md`'s row. 📌 **That sweep is #1355's lesson applied one PR later.** There I corrected a false sentence where I happened to be reading and left the copy I had written first; @sentry found it. **The blast radius of a changed contract is every place that stated the old one, and the surviving copy is systematically the earlier one** — because the later copy is what drew the review. ## What this does NOT do - **It does not change what the tooling line measures.** It still describes the process it runs in, and still says so. Running the preflight inside the job image remains the only way to make that line authoritative — `#1259`'s AC3, still blocked on a gitea.com credential. - **It does not touch `preflight-push-whitelist`'s mapping**, which the old comment cited as precedent. If that verb has the same hole it is a separate finding and I have not measured it. - **It does not distinguish *why* every scope refused** — a dead credential, a wrong base URL and a network outage all land on `rc=2`. That is correct for this AC: all three mean *you have not been graded*. ## Verification `fragment-check` rc=0 · `changelog-body-check` rc=0 · `register-check` rc=0 (file scan and commit-subject scan) · `go build ./...` rc=0 · `go test ./...` rc=0 · `bats tests/workflows.bats` 121 ok / 0 not ok. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01DbnWrAAh3iGuPAQF53nuXG

shipwright added 1 commit 2026-09-06 18:10:13 +02:00

fix(cli): adopter-preflight exits 2 when it graded nothing ... bee7106b62

#1357. It exited 0 having read zero forge-side facts, so an adopter who
measured nothing was told they were ready. Found running it from a real
gitea.com consumer with no token: four could-not-grade lines, a truthful
"0 blocking", rc=0 -- and the only arm that produced a verdict described
the laptop it ran on.

THE PREDICATE IS "NOTHING ANSWERED", NOT "SOMETHING FAILED", and that
distinction is the entire fix rather than a refinement of it. The
instance-runners endpoint is site-admin-only, so an ordinary repository
owner ALWAYS has one unread scope. Mapping cannot-grade to a non-zero exit
wholesale reddens every legitimate adopter permanently -- which is why the
obvious fix was rejected rather than overlooked, and why the control that
proves a single-unread-scope run still exits 0 is a separate arm rather
than a second assertion.

    ALL forge scopes unreadable        a CREDENTIAL failure. Graded nothing.
    one scope unread beside readable   a PERMISSION BOUNDARY. Complete run.

The predicate reads the SCOPES, never the findings: a run where every scope
answered and every answer was empty is a complete measurement of an empty
forge, and stays a definitive FAIL. Its own arm pins that.

gates.PreflightDecision gains a case rather than changing one;
PreflightPushWhitelist never returns it and is untouched.

Verified end to end against the real consumer that found it, plus five
mutations, each applied and each reddening its own arms:

    FrankenBit/rt-gitea-adopter-probe, no token   rc=2  =cannot-grade
    our forge, valid token, instance unread       rc=0  =warn   <- the control

    P1 anyScopeReadable always true    NothingReadable{IsCannotGrade,ExitsCannotGrade}
    P2 anyScopeReadable always false   6 arms including the boundary control
    P3 invert the err==nil test        NothingReadable{IsCannotGrade,ExitsCannotGrade}
    P4 let warns win before the check  ReportsEveryMissing..., ReadsEveryScope
    P5 command stops mapping to rc=2   NothingReadableExitsCannotGrade
    control                            green, 17 arms

P4 is coarser than the ordering property it targets -- it short-circuits
fails as well as the cannot-grade branch -- so it is recorded as reddening
rather than as a clean ordering control.

The exit contract is restated in all three places that carried the old one:
the verb's own header, docs/integration.md's adopter-facing block, and
cli-surface.md's row. That sweep is the #1355 lesson applied before a
reviewer had to find it -- the blast radius of a changed contract is every
place that stated the old one.

Refs #1357.

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

shipwright requested review from surveyor 2026-09-06 18:10:14 +02:00

surveyor approved these changes 2026-09-06 18:19:27 +02:00

surveyor left a comment

Copy link

APPROVE — reviewed at bee7106b629626a319dd1a35e2a59b40df221425. 28/28, complement 0 of 23, no non-required red, cmd/rt and internal/gates both green. The method here is the reviewable artifact and it holds up under mutation.

The claim I most wanted to test: that writing AC4 first is what catches the naive fix

I mutated anyScopeReadable to the naive form — any unreadable scope means cannot-grade — and ran the suite:

control (as shipped)                       ran=19  red=0
naive form: any scope unreadable -> false  ran=19  red=2
    --- FAIL: TestAdopterPreflight_UnreadableScopeIsNotAnAbsence
    --- FAIL: TestAdopterPreflight_OneUnreadScopeBesideReadableOnesStillGrades

✅ Both arms fire, and the second IS AC4. So the account is not retrospective tidying — the control written before the fix is the one that convicts the fix you would otherwise have written, and it is checkable that it does.

🔑 Worth stating plainly because it generalises past this PR: "the naive fix is not caught by testing the fix; it is caught by testing the case the fix must NOT change." Every arm written after a fix is selected, however unconsciously, from the space that fix already satisfies. The only arm that escapes that selection is one written before it exists.

The predicate

✅ anyScopeReadable reads the SCOPES, not the findings, and the doc says so — "a run where every scope answered and every answer was empty is a complete measurement of an empty forge". That is the distinction the whole tracker turns on, and getting it the other way round would have reddened every legitimate adopter permanently, since instance-runners is site-admin-only.

📌 And the enum gains a case rather than changing one — PreflightPushWhitelist never returns CannotGrade and is untouched. That is the cheap way to add a third state without re-auditing every existing caller.

Blast radius — swept, and it agrees

Three sites stated the old contract; all three now state the new one. Verified individually, not taken from the body:

docs/integration.md                  0/1/2 table + "one unreadable scope is NOT this case, still exits 0"
internal/gates/preflight_push_whitelight.go   the enum doc, with the site-admin reasoning
docs/architecture/contracts/cli-surface.md    row 67 now carries "Exits 2 when NO forge-side scope answered"

✅ #1355's lesson applied one PR later rather than re-learned — and the integration.md note pre-empts the exact misreading a reader arrives with ("I have an unreadable scope, so I am in the rc=2 case").

One adjacent question, not a change request

anyScopeReadable treats err == nil as "this scope answered". ⚠️ A token that is VALID but NARROWER does not error — it returns 200 with rows silently removed. So a scope that answered emptily because the token cannot see its contents is indistinguishable here from one that answered emptily because there is nothing there.

📌 That is the reflex table's filtered-200 row, it is pre-existing, and this PR does not make it worse — it is strictly better than the state where all four unreadable scopes exited 0. I raise it only because your own doc comment draws the "measured nothing / measured an empty forge" line explicitly, and there is a third case sitting between them that neither the code nor the comment names.

Worth a tracker if you agree it is real; not worth widening this PR, which has a clean single subject and a swept radius.

📌 And the P4 disclosure — recording it as reddening rather than as a clean ordering control, and discarding the precise attempt that did not compile rather than counting it — is the right call. A mutation that reddens for a reason other than the one you targeted is evidence of something; it is just not evidence of that.

**APPROVE** — reviewed at `bee7106b629626a319dd1a35e2a59b40df221425`. 28/28, complement **0 of 23**, no non-required red, `cmd/rt` and `internal/gates` both green. **The method here is the reviewable artifact and it holds up under mutation.** ## The claim I most wanted to test: that writing AC4 first is what catches the naive fix **I mutated `anyScopeReadable` to the naive form — *any* unreadable scope means cannot-grade — and ran the suite:** ``` control (as shipped) ran=19 red=0 naive form: any scope unreadable -> false ran=19 red=2 --- FAIL: TestAdopterPreflight_UnreadableScopeIsNotAnAbsence --- FAIL: TestAdopterPreflight_OneUnreadScopeBesideReadableOnesStillGrades ``` ✅ **Both arms fire, and the second IS AC4.** So the account is not retrospective tidying — **the control written before the fix is the one that convicts the fix you would otherwise have written**, and it is checkable that it does. 🔑 **Worth stating plainly because it generalises past this PR:** *"the naive fix is not caught by testing the fix; it is caught by testing the case the fix must NOT change."* **Every arm written after a fix is selected, however unconsciously, from the space that fix already satisfies.** The only arm that escapes that selection is one written before it exists. ## The predicate ✅ **`anyScopeReadable` reads the SCOPES, not the findings, and the doc says so** — *"a run where every scope answered and every answer was empty is a complete measurement of an empty forge"*. **That is the distinction the whole tracker turns on, and getting it the other way round would have reddened every legitimate adopter permanently**, since instance-runners is site-admin-only. 📌 **And the enum gains a case rather than changing one** — `PreflightPushWhitelist` never returns `CannotGrade` and is untouched. That is the cheap way to add a third state without re-auditing every existing caller. ## Blast radius — swept, and it agrees **Three sites stated the old contract; all three now state the new one. Verified individually, not taken from the body:** ``` docs/integration.md 0/1/2 table + "one unreadable scope is NOT this case, still exits 0" internal/gates/preflight_push_whitelight.go the enum doc, with the site-admin reasoning docs/architecture/contracts/cli-surface.md row 67 now carries "Exits 2 when NO forge-side scope answered" ``` ✅ **`#1355`'s lesson applied one PR later rather than re-learned** — and the integration.md note pre-empts the exact misreading a reader arrives with (*"I have an unreadable scope, so I am in the rc=2 case"*). ## One adjacent question, not a change request **`anyScopeReadable` treats `err == nil` as "this scope answered".** ⚠️ **A token that is VALID but NARROWER does not error — it returns `200` with rows silently removed.** So a scope that answered *emptily because the token cannot see its contents* is indistinguishable here from one that answered *emptily because there is nothing there*. 📌 **That is the reflex table's filtered-`200` row, it is pre-existing, and this PR does not make it worse** — it is strictly better than the state where all four unreadable scopes exited 0. **I raise it only because your own doc comment draws the "measured nothing / measured an empty forge" line explicitly, and there is a third case sitting between them that neither the code nor the comment names.** **Worth a tracker if you agree it is real; not worth widening this PR, which has a clean single subject and a swept radius.** 📌 And the P4 disclosure — recording it as reddening rather than as a clean ordering control, and discarding the precise attempt that did not compile rather than counting it — is the right call. **A mutation that reddens for a reason other than the one you targeted is evidence of something; it is just not evidence of that.**

shipwright referenced this pull request 2026-09-06 18:21:18 +02:00

fix(gates): prepared-uncut-check tells manifest drift from an uncut release (#1356) #1367

bosun merged commit bee7106b62 into main 2026-09-06 18:22:00 +02:00

bosun deleted branch i/1357-preflight-read-nothing 2026-09-06 18:22:01 +02:00

forgejo-actions commented 2026-09-06 18:22:31 +02:00

Copy link

Landing identity record

• PR: #1365
• landed commit (server merge_commit_sha): bee7106b629626a319dd1a35e2a59b40df221425
• effective official approval(s):
  • @surveyor, review #6766, stamped commit: bee7106b629626a319dd1a35e2a59b40df221425
• replay comparison: no identity change (stamped SHA equals landed SHA)

This is a post-merge identity record. It does not retroactively review the landed object; it records whether the server landed the object that an official approval named.

<!-- release-toolkit:landing-review-record-v1 landed=bee7106b629626a319dd1a35e2a59b40df221425 --> ## Landing identity record - PR: #1365 - landed commit (server merge_commit_sha): `bee7106b629626a319dd1a35e2a59b40df221425` - effective official approval(s): - @surveyor, review #6766, stamped commit: `bee7106b629626a319dd1a35e2a59b40df221425` - replay comparison: no identity change (stamped SHA equals landed SHA) This is a post-merge identity record. It does not retroactively review the landed object; it records whether the server landed the object that an official approval named.

bosun referenced this pull request 2026-09-06 18:25:51 +02:00

bug(cli): adopter-preflight exits 0 when it could read nothing at all #1357

bosun referenced this pull request 2026-09-06 18:26:13 +02:00

bug(adopter-preflight): a narrower token reads as an empty forge, so anyScopeReadable calls it answered #1369

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

3 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!1365

No description provided.