bug(cli): review-eligibility answers ELIGIBLE when the forge never mentioned the whitelist #1342

New issue

Closed

opened 2026-09-06 16:07:47 +02:00 by bosun · 0 comments

bosun commented

2026-09-06 16:07:47 +02:00

Owner

rt review-eligibility answers ELIGIBLE out of a forge that never mentioned the field, because EnableApprovalsWhitelist is a plain bool and an absent field decodes to false.

Found by @quartermaster reviewing #1339, which implements #1277. Not blocking; he approved and it merged at 7c03a86b.

The defect

if !bp.EnableApprovalsWhitelist { ELIGIBLE (approval whitelist disabled) }

A plain bool cannot distinguish "the forge said false" from "the forge said nothing". An erroring read is rc=2 correctly; a successful 200 with the field absent produces a confident positive.

The function's own doc comment states the rule this breaks: "A missing/unreadable input is rc=2; it must never collapse into an empty allowlist and produce a confident negative or positive." It holds for every input except this one.

Why it matters more than a normal nil-decode

#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 exactly that, through the tool built to prevent it.

The fix, and the precedent is in the same struct

EnablePush and PushWhitelistDeployKeys are both *bool ten lines above, from #1313/#1105. The rationale written there substitutes unchanged: "far too strong a claim to manufacture out of a forge that never mentioned it." Roughly 5 lines.

Measured

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, which is the argument FOR the pointer rather than against it.

Why this was not a blocking review

The field is present on this forge, nothing consumes the command yet, and a false ELIGIBLE fails toward a stamp that does not count rather than a bad merge — the gate still holds. Drift-resistance, not a live defect. Filed rather than fixed inline because pushing to #1339 would have dismissed its only approval (dismiss_stale_approvals=true) to buy five lines.

AC

EnableApprovalsWhitelist is *bool, and a nil read is rc=2 rather than ELIGIBLE. The refusal comes BEFORE any identity read — reading identity to answer a question the forge never posed is the same overreach one layer down.
An arm covers a 200 whose body omits the field, and reddens on its own mutation. Two arms, different halves: internal/forgejo pins that an omitted key decodes nil while explicit false decodes non-nil false — the distinction the pointer exists for; cmd/rt pins rc=2, empty stdout, no identity call. Mutation (collapse the nil branch into the false branch): applied=YES rc=1, restored rc=0, applied= read off the file.

`rt review-eligibility` answers ELIGIBLE out of a forge that never mentioned the field, because `EnableApprovalsWhitelist` is a plain bool and an absent field decodes to false. Found by @quartermaster reviewing `#1339`, which implements `#1277`. Not blocking; he approved and it merged at `7c03a86b`. ## The defect ```go if !bp.EnableApprovalsWhitelist { ELIGIBLE (approval whitelist disabled) } ``` A plain `bool` cannot distinguish "the forge said false" from "the forge said nothing". An erroring read is `rc=2` correctly; a successful 200 with the field absent produces a confident positive. The function's own doc comment states the rule this breaks: *"A missing/unreadable input is rc=2; it must never collapse into an empty allowlist and produce a confident negative or positive."* It holds for every input except this one. ## Why it matters more than a normal nil-decode `#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 exactly that, through the tool built to prevent it.** ## The fix, and the precedent is in the same struct `EnablePush` and `PushWhitelistDeployKeys` are both `*bool` ten lines above, from `#1313`/`#1105`. The rationale written there substitutes unchanged: *"far too strong a claim to manufacture out of a forge that never mentioned it."* Roughly 5 lines. ## Measured `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, which is the argument FOR the pointer rather than against it. ## Why this was not a blocking review The field is present on this forge, nothing consumes the command yet, and a false ELIGIBLE fails toward a stamp that does not count rather than a bad merge — the gate still holds. Drift-resistance, not a live defect. Filed rather than fixed inline because pushing to `#1339` would have dismissed its only approval (`dismiss_stale_approvals=true`) to buy five lines. ## AC - [x] `EnableApprovalsWhitelist` is `*bool`, and a nil read is `rc=2` rather than ELIGIBLE. **The refusal comes BEFORE any identity read** — reading identity to answer a question the forge never posed is the same overreach one layer down. - [x] An arm covers a 200 whose body omits the field, and reddens on its own mutation. **Two arms, different halves:** `internal/forgejo` pins that an omitted key decodes `nil` while explicit `false` decodes non-nil `false` — the distinction the pointer exists for; `cmd/rt` pins `rc=2`, empty stdout, no identity call. Mutation (collapse the nil branch into the false branch): `applied=YES rc=1`, restored `rc=0`, `applied=` read off the file.