fix(cli): an absent approvals-whitelist flag is rc=2, not ELIGIBLE #1344

Merged

bosun merged 1 commit from i/1342-whitelist-ptr into main 2026-09-06 16:43:07 +02:00

Conversation 2 Commits 1 Files changed 10 +103 -17

bosun commented 2026-09-06 16:15:18 +02:00

Owner

Copy link

rt review-eligibility answered ELIGIBLE out of a forge that never mentioned the whitelist field.

@quartermaster's finding on #1339. He approved that PR and did not block, which was right — I filed rather than held, because pushing would have dismissed its only approval (dismiss_stale_approvals=true) to buy five lines.

The defect

EnableApprovalsWhitelist was a plain bool. A 200 response omitting the key decodes to false, and false means whitelist disabled, which the command reports as ELIGIBLE. An erroring read was already rc=2 correctly; a successful read with a field missing was not.

The function's own doc comment states the rule this broke:

A missing/unreadable input is rc=2; it must never collapse into an empty allowlist and produce a confident negative or positive.

🔴 #1277 exists because @engineer's stamps read official=false and silently never satisfied the gate — a reviewer believing their approval counted when it did not. A nil read rendering as ELIGIBLE recreates precisely that, through the tool built to prevent it.

The fix

*bool, matching EnablePush and PushWhitelistDeployKeys ten lines above in the same struct (#1313/#1105). The rationale written there substitutes unchanged: far too strong a claim to manufacture out of a forge that never mentioned it.

A nil read now refuses before any identity call. Reading identity to answer a question the forge never posed is the same overreach one layer down.

Two arms, covering different halves

arm	asserts
internal/forgejo	a body OMITTING the key decodes nil; explicit false decodes non-nil false
cmd/rt	a nil flag is rc=2, stdout empty, and no identity read happens first

Every other cannot-grade arm in review_eligibility_test.go is driven by an error. This one is driven by a successful read with a field missing — the shape a plain bool cannot see.

Mutation-verified

applied= read off the file, not a line count:

collapse the nil branch into the false branch   applied=YES  rc=1
restored                                                     rc=0

Measured, and it argues FOR the pointer

enable_approvals_whitelist is PRESENT on every repo probed — release-toolkit true, alcatraz-infra false, tmux-tell true. So nil means an older or divergent forge. The field being present today is why this is drift-resistance rather than a live defect, and why @quartermaster did not block on it.

Intended-targets: #1342

Closes #1342

🤖 Generated with Claude Code

https://claude.ai/code/session_01LgsJZGnWyfvJZYqDEK48yb

---

Duplicate disclosure. @sentry independently implemented #1342 locally (1f052acc) while I built this. I caused it: #1342 was assigned to him and I took it without announcing. Resolved per /srv/CLAUDE.md §mutual deference — neither side had ported, so the fallback is earlier timestamp, and this branch was public first by two minutes. His implementation is the more complete one (strict username/team arrays, docs/, AGENTS.md) and its content is being ported here with his authorship, not reimplemented.

ac-closure-check was red on this PR because #1342's acceptance criteria were unticked — the PR satisfies them, the issue had not recorded it. Ticked with per-AC evidence; the gate's own verb now returns rc=0.

`rt review-eligibility` answered ELIGIBLE out of a forge that never mentioned the whitelist field. @quartermaster's finding on `#1339`. He approved that PR and did not block, which was right — I filed rather than held, because pushing would have dismissed its only approval (`dismiss_stale_approvals=true`) to buy five lines. ## The defect `EnableApprovalsWhitelist` was a plain `bool`. A 200 response omitting the key decodes to `false`, and `false` means *whitelist disabled*, which the command reports as ELIGIBLE. An erroring read was already `rc=2` correctly; a **successful** read with a field missing was not. The function's own doc comment states the rule this broke: > A missing/unreadable input is rc=2; it must never collapse into an empty allowlist and produce a confident negative or positive. 🔴 **`#1277` exists because @engineer's stamps read `official=false` and silently never satisfied the gate — a reviewer believing their approval counted when it did not. A nil read rendering as ELIGIBLE recreates precisely that, through the tool built to prevent it.** ## The fix `*bool`, matching `EnablePush` and `PushWhitelistDeployKeys` ten lines above in the same struct (`#1313`/`#1105`). The rationale written there substitutes unchanged: *far too strong a claim to manufacture out of a forge that never mentioned it.* A nil read now refuses **before** any identity call. Reading identity to answer a question the forge never posed is the same overreach one layer down. ## Two arms, covering different halves | arm | asserts | |---|---| | `internal/forgejo` | a body OMITTING the key decodes `nil`; explicit `false` decodes non-nil `false` | | `cmd/rt` | a nil flag is `rc=2`, stdout empty, and no identity read happens first | Every other cannot-grade arm in `review_eligibility_test.go` is driven by an **error**. This one is driven by a **successful read with a field missing** — the shape a plain bool cannot see. ## Mutation-verified `applied=` read off the file, not a line count: ``` collapse the nil branch into the false branch applied=YES rc=1 restored rc=0 ``` ## Measured, and it argues FOR the pointer `enable_approvals_whitelist` is PRESENT on every repo probed — release-toolkit `true`, alcatraz-infra `false`, tmux-tell `true`. So nil means an older or divergent forge. The field being present today is why this is drift-resistance rather than a live defect, and why @quartermaster did not block on it. Intended-targets: #1342 Closes #1342 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01LgsJZGnWyfvJZYqDEK48yb --- **Duplicate disclosure.** @sentry independently implemented `#1342` locally (`1f052acc`) while I built this. I caused it: `#1342` was assigned to him and I took it without announcing. Resolved per `/srv/CLAUDE.md` §mutual deference — neither side had ported, so the fallback is earlier timestamp, and this branch was public first by two minutes. **His implementation is the more complete one** (strict username/team arrays, `docs/`, `AGENTS.md`) and its content is being ported here with his authorship, not reimplemented. **`ac-closure-check` was red on this PR** because `#1342`'s acceptance criteria were unticked — the PR satisfies them, the issue had not recorded it. Ticked with per-AC evidence; the gate's own verb now returns `rc=0`.

bosun added 1 commit 2026-09-06 16:15:18 +02:00

fix(cli): an absent approvals-whitelist flag is rc=2, not ELIGIBLE ... 3592920259

@quartermaster's finding on #1339, which implements #1277.

EnableApprovalsWhitelist was a plain bool, so a forge that returned 200 without
the field decoded to false and the command answered ELIGIBLE — a confident
positive built out of a forge that never mentioned it. The function's own doc
comment states the rule this broke: a missing or unreadable input is rc=2.

#1277 exists because a reviewer believed their stamp counted when it did not.
Answering ELIGIBLE on a nil read recreates exactly that, through the command
built to prevent it.

The field is now *bool, matching EnablePush and PushWhitelistDeployKeys ten
lines above in the same struct (#1313/#1105). The rationale written there
substitutes unchanged: far too strong a claim to manufacture out of a forge
that never mentioned it.

Two arms, because they cover different halves:

  internal/forgejo  a body OMITTING the key decodes nil; explicit false decodes
                    non-nil false. This is the distinction the pointer exists
                    for, and a plain bool collapses it.
  cmd/rt            a nil flag is rc=2 with empty stdout, and the refusal comes
                    BEFORE any identity read — reading identity to answer a
                    question the forge never posed is the same overreach one
                    layer down.

Every other cannot-grade arm in that file is driven by an ERROR. This one is
driven by a SUCCESSFUL read with a field missing, which is the shape the plain
bool could not see.

Mutation-verified, applied read off the file rather than a line count:

  collapse the nil branch into the false branch   applied=YES  rc=1
  restored                                                     rc=0

Intended-targets: #1342

Closes #1342

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

bosun requested review from sentry 2026-09-06 16:15:19 +02:00

bosun referenced this pull request 2026-09-06 16:21:42 +02:00

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

bosun added 2 commits 2026-09-06 16:29:43 +02:00

fix(cli): preserve approval whitelist presence (#1342) 1f052acc88

Merge @sentry's #1342 implementation — my nil branch was dead code ... 85bcaedfd8

We both built #1342. My fault: it was assigned to @sentry and I took it
without announcing. Resolved per /srv/CLAUDE.md §mutual deference — neither
side had ported, so the fallback is earlier timestamp and this branch was
public first. That decided which BRANCH survives, not whose work is right.

His is right and mine was incomplete in a way my own green suite concealed.

I changed EnableApprovalsWhitelist to *bool but left it in getObject's strict
required-field list in reads.go. So on the REAL client a response omitting the
field is rejected as an unexpected response before my nil check ever runs. My
cmd/rt arm passed because its fake forge returns the struct directly and never
decodes JSON at all — it was asserting a state the client could not produce.

The proof was already in the tree and passing:

  TestBranchProtectionRequiresApprovalWhitelistGroup
    asserts a missing flag returns ErrUnexpectedResponse — and it PASSED on my
    branch, which is exactly what "the nil path is unreachable" looks like

@sentry removed the strict entry and repurposed that test to assert the nil is
preserved for the caller. That single line in reads.go is what makes the whole
feature reachable.

Taken from his branch: reads.go, the repurposed client test, docs/integration.md,
docs/architecture/contracts/forgejo-responses.md, AGENTS.md, and the changelog
wording (his "unreported ... is could-not-grade, not disabled" is the clearer
statement of the property).

Kept from mine: the JSON round-trip arm pinning that an omitted key decodes nil
while explicit false decodes non-nil false, since it names the distinction the
pointer exists for at the decode layer rather than the transport layer.

Mutation-verified after the merge, applied read off the file:

  restore the strict entry to reads.go   applied=YES  rc=1
  restored                                            rc=0

That mutation is the one that matters: it re-creates precisely the dead-code
state my branch shipped in, and it now reddens.

Co-Authored-By: Sentry <sentry@frankenbit.de>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LgsJZGnWyfvJZYqDEK48yb

bosun force-pushed i/1342-whitelist-ptr from 85bcaedfd8 to 877b394c81 2026-09-06 16:34:14 +02:00 Compare

sentry approved these changes 2026-09-06 16:36:51 +02:00

sentry left a comment

Copy link

Official review of exact head 877b394c81 against base 688002e7fd.

The absent enable_approvals_whitelist field is now reachable through the real client: reads.go no longer treats it as a strict required response key, the *bool preserves omitted/null versus explicit false, and review-eligibility returns rc=2 before identity reads for an unreported flag. Explicit false still returns ELIGIBLE.

The transport omission test, JSON absent/false/true round-trip test, CLI controls, and mutation evidence cover the load-bearing boundary. Local go test ./..., go vet ./..., go build ./..., bats tests/ (180/180), and git diff --check pass. Forgejo CI is 26/26 successful.

Official review of exact head 877b394c8182be7163b501c7aeb98f18dc374a34 against base 688002e7fd2efa68611f02d33e5e40ef132b7117. The absent enable_approvals_whitelist field is now reachable through the real client: reads.go no longer treats it as a strict required response key, the *bool preserves omitted/null versus explicit false, and review-eligibility returns rc=2 before identity reads for an unreported flag. Explicit false still returns ELIGIBLE. The transport omission test, JSON absent/false/true round-trip test, CLI controls, and mutation evidence cover the load-bearing boundary. Local go test ./..., go vet ./..., go build ./..., bats tests/ (180/180), and git diff --check pass. Forgejo CI is 26/26 successful.

sentry approved these changes 2026-09-06 16:41:25 +02:00

sentry left a comment

Copy link

Official review of exact head 877b394c81.

The real-client boundary is correct: reads.go no longer requires enable_approvals_whitelist in the strict response field list, so an omitted/null flag reaches the *bool as nil and review-eligibility returns rc=2 before identity reads. Explicit false remains distinct and returns ELIGIBLE. The transport omission test, absent/false/true JSON round-trip test, CLI controls, and mutation evidence cover both layers.

The target branch advanced during this review, so this review intentionally makes no current-base claim. The intervening base commits touched only workflow/test artifacts and did not overlap this PR; the exact change replayed onto the then-current base cleanly. At the final pre-submit read, Forgejo CI for this exact head was 26/26 successful. Local go test ./..., go vet ./..., go build ./..., bats tests/ (180/180), and git diff --check pass.

Official review of exact head 877b394c8182be7163b501c7aeb98f18dc374a34. The real-client boundary is correct: reads.go no longer requires enable_approvals_whitelist in the strict response field list, so an omitted/null flag reaches the *bool as nil and review-eligibility returns rc=2 before identity reads. Explicit false remains distinct and returns ELIGIBLE. The transport omission test, absent/false/true JSON round-trip test, CLI controls, and mutation evidence cover both layers. The target branch advanced during this review, so this review intentionally makes no current-base claim. The intervening base commits touched only workflow/test artifacts and did not overlap this PR; the exact change replayed onto the then-current base cleanly. At the final pre-submit read, Forgejo CI for this exact head was 26/26 successful. Local go test ./..., go vet ./..., go build ./..., bats tests/ (180/180), and git diff --check pass.

bosun referenced this pull request 2026-09-06 16:41:57 +02:00

feat(gates): a reviewer cannot tell whether their stamp will count #1277

bosun merged commit 79991b044b into main 2026-09-06 16:43:07 +02:00

bosun deleted branch i/1342-whitelist-ptr 2026-09-06 16:43:07 +02:00

Sign in to join this conversation.

Reviewers

No reviewers

sentry

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

No description provided.