fix(cli): an absent approvals-whitelist flag is rc=2, not ELIGIBLE #1344
No reviewers
Labels
No labels
bump
major
bump
minor
bump
patch
kind
bug
kind
chore
kind
docs
kind
epic
kind
feature
loop/paused
mirror-drift
priority
critical
priority
high
priority
low
priority
medium
size
L
size
M
size
S
size
XL
status
blocked
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
frankenbit/release-toolkit!1344
Loading…
Reference in a new issue
No description provided.
Delete branch "i/1342-whitelist-ptr"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
rt review-eligibilityanswered 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
EnableApprovalsWhitelistwas a plainbool. A 200 response omitting the key decodes tofalse, andfalsemeans whitelist disabled, which the command reports as ELIGIBLE. An erroring read was alreadyrc=2correctly; a successful read with a field missing was not.The function's own doc comment states the rule this broke:
🔴
#1277exists because @engineer's stamps readofficial=falseand 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, matchingEnablePushandPushWhitelistDeployKeysten 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
internal/forgejonil; explicitfalsedecodes non-nilfalsecmd/rtrc=2, stdout empty, and no identity read happens firstEvery other cannot-grade arm in
review_eligibility_test.gois 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:Measured, and it argues FOR the pointer
enable_approvals_whitelistis PRESENT on every repo probed — release-toolkittrue, alcatraz-infrafalse, tmux-telltrue. 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
#1342locally (1f052acc) while I built this. I caused it:#1342was 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-checkwas 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 returnsrc=0.85bcaedfd8877b394c81Official review of exact head
877b394c81against base688002e7fd.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
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.