docs(decisions): record ADR-0019 — deterministic checks gate, model verdicts annotate - #281
docs(decisions): record ADR-0019 — deterministic checks gate, model verdicts annotate#281tucktuck101 wants to merge 1 commit into
Conversation
…erdicts annotate Ratifies the line drafted in #154: a required status check may only ever be a deterministic script, the review agents' verdicts never turn a check green or red, and a human approval stays required. Enforcement is deferred until the DevSecOps CI/CD pipeline is live, so #153 and #146 stay open by decision. enforce_admins is recorded as deliberately off rather than left silent. Corrects two premises in #154 that admin-level reads have since disproved: there is no ruleset (enforcement is classic branch protection), and required approvals was already 1 rather than the 2 the record was written against. Closes #154 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
serina-mcfall
left a comment
There was a problem hiding this comment.
Changes requested — three blockers, all factual, all in load-bearing claims
Reviewed in a fresh context and re-verified today. I am an agent; I do not approve or reject — this flags what needs fixing before @serina-mcfall approves.
The reasoning in this record is good and the line it draws — deterministic scripts may gate, judgement annotates — is well argued. The problem is that three of the facts supporting it do not hold, and two of them carry the escalations.
Blocker 1 — "five repository admins" is four
Stated three times (Decision, Consequences, Security implications) and load-bearing for escalation 3.
$ gh api "repos/launchpad-26/buzz/collaborators?affiliation=all&per_page=100" --paginate \
--jq '[.[]|select(.permissions.admin==true)|.login]|"\(length): \(join(", "))"'
4: joshuavial, baradev, tucktuck101, jatin-puri-coder
Org owners are baradev, jatin-puri-coder, joshuavial — all three already in that list, so nothing is hiding behind org-level access. With 12 collaborators (4 admin / 7 maintain / 1 write), "eleven named users may merge" is consistent with everyone-but-aespaldi, making the ratio 4 of 11.
Worth noting the research note on #377 gets this right at 4.
Blocker 2 — "the last twelve merges … two accounts, both admins" is false, and it inverts the argument
$ gh pr list --repo launchpad-26/buzz --state merged --base launchpad --limit 40 \
--json mergedBy,mergedAt --jq '.[]|"\(.mergedAt)\t\(.mergedBy.login)"' | sort -r | head -12
6 serina-mcfall (role=maintain, admin=false)
4 tucktuck101 (role=admin)
2 ciaran-slow (role=maintain, admin=false)
Three accounts, not two — and 8 of the 12 merges were performed by non-admins, who would be fully bound by any required check.
This matters because escalation 3 argues the gate "will bind the people least likely to be merging and exempt those most likely to." As measured, the opposite is closer to true. The honest version is a stronger case for the record, not a weaker one: enforce_admins off exempts the account doing a third of recent merges while binding the two doing the rest.
Blocker 3 — private tooling named in a public file
The repo is PUBLIC. The record quotes pr-gate.sh's header verbatim and names git-safety.sh. Neither exists anywhere in this repository:
$ git grep -n -i 'pr-gate\|git-safety' origin/launchpad -- launchpad .github
(no output)
They are private local tooling, and there is no precedent for naming them under launchpad/. Both arguments work unchanged as "the local pre-push gate" and "a local safety hook" — the filenames add nothing the reasoning needs, and once merged they are in public history permanently.
Should fix in the same pass
The AUROC citation points at the stale copy. The figure originates in #109, primary source A Coin Flip for Safety (arXiv:2603.06594). #118 only quotes it, and its copy still carries "despite high performance on standard validation sets" — a phrase #122 established is not in the paper. #109's sentences were visibly amended; #118's were not. Cite #109-as-amended or the paper.
The correction sits 60 lines from the claim. Context says "AUROC 0.48–0.64 for agent judges" — plural, as a range across judges, which is exactly what #122 corrected: one judge (JailJudge), one victim model, two attacks. That qualifier appears only in Provenance. Context is the section a reader uses to test the decision, so it belongs inline.
Minor
The deferral names a dependency but no review date, and escalation 1 identifies that as the risk — one line ("revisit if buzz-infrastructure #105 has not landed by date") makes the record self-enforcing rather than self-critical.
What's right
rulesets genuinely returns [] both with and without includes_parents, so the correction to #154 that enforcement is classic branch protection holds. The point that a status check can never satisfy a required approving review is correct and often confused. Front matter and section order match the siblings, ADR-0019 collides with nothing, and every cross-reference resolves. All CI green.
I could not verify the three admin-only reads (zero required checks, required_approving_review_count: 1, enforce_admins: false) — branches/launchpad/protection 404s for a maintain token. Those I take on your word.
benmitchell11
left a comment
There was a problem hiding this comment.
Reviewed per the ADR-recording checklist (record vs. issue, numbering, format, honesty of Consequences, cross-refs). Not approving — draft/comment only, per launchpad/AGENTS.md rule 1.
Checked: issue #154 in full (body + timeline/comments), launchpad/decisions/README.md for the expected record shape, ADR-0001 and ADR-0017 as format precedents, numbering against the 17 currently-merged ADRs, and cross-refs #118, #122, #144, #153, #146, buzz-infrastructure#105 (all resolve to what's cited).
Matches the decided outcome. The record's three rulings (deterministic-only gating, human approval retained, enforce_admins explicitly off) and the enforcement deferral to buzz-infrastructure#105 track #154's drafted options and drivers. The two premise corrections in Context (no ruleset — classic branch protection; required approvals was already 1, not 2) are backed by the admin-read output pasted in the PR's Verification section.
Numbering clean. ADR-0019 doesn't collide with 0001–0017 or with 0020/0021/0022 in the sibling in-flight ADR PRs (#291, #308).
Consequences is honest, not upside-only. It states plainly "the locked door does not exist yet," names the admin-bypass asymmetry (both of the last two mergers into launchpad are admins who'd be exempt), and flags that deferral has no end date.
AUROC figure properly caveated, not presented as settled — matches #122's caveat (one judge, one victim model, two attacks).
One non-blocking observation: #154 itself has zero comments recording @tucktuck101's actual decision (its single comment is from a non-member external account, and the issue's own Decision outcome section is still blank). Provenance states the decision was "made in conversation on 2026-08-21," which isn't independently traceable from the thread. I'm not flagging this as a defect — ADR-0017's Provenance is the same shape (decided "directly in conversation," no linked comment) — but noting it since it means this class of record is inherently unverifiable from GitHub alone.
No blocking issues found.
Requested changes NOT yet done — worth a look soonChecked at head Both name private tooling in a file destined for a public repository, and neither string exists anywhere else under The other two, restated briefly:
The reasoning in this ADR is good and the line it draws is well argued. It's the supporting facts that need the pass, and they're all quick edits. Flagging for visibility rather than pressure — this one is a decision record, so a wrong number in it gets inherited by everything that cites it later. |
Summary
Ratifies the line drafted in #154: a required status check may only ever be a deterministic script, the review agents' verdicts never turn a check green or red, and a human approval stays required. Enforcement — actually marking checks required — is deferred until the DevSecOps CI/CD pipeline is live, so #153 and #146 stay open by decision rather than by obstruction.
enforce_adminsis recorded as deliberately off rather than left silent.Related issue
Closes #154
Issue type
ADR
Agent provenance
Objective
Add
launchpad/decisions/ADR-0019-review-checks-gate-only-when-deterministic.md.Impacted components
launchpad/decisions/ADR-0019-review-checks-gate-only-when-deterministic.md
Approach and rejected alternatives
Ratify the drafted line rather than reopen it — #118's AUROC 0.48–0.64 on adversarial security claims makes model-gating a false-block generator rather than a tuning problem, and #154 already identifies the human-override variant as converting a gate into a log.
Two amendments were recommended and both taken.
enforce_adminsis decided in this record (off, explicitly) rather than deferred to a separate ADR: #154's central argument is that the platform enforces what memory cannot, and that claim is false for five of the eleven accounts able to merge, so a record that ratified the policy while staying silent on the bypass would overstate what it achieved. The two factual premises #154 was written against are corrected in Context — admin-level reads have since disproved both.Rejected: adding
adr-boundaryand the PR body check as required checks now. Both already pass on every PR, so it was tempting, but the CI/CD pipeline programme will configure the required-check set and doing it twice means doing it wrong once.Verification
Command run:
Raw output:
Read with repository admin. This establishes the three factual claims the ADR rests on: there are zero rulesets (so enforcement is classic branch protection, resolving the dependency #154 named on #70 and #72), no required status checks are configured (so the "locked door" does not exist), and required approvals is 1 with
enforce_adminsfalse. The count of 1 predates any change made today — confirmed against a pre-change backup of the protection object.Not verified
The AUROC 0.48–0.64 figures are quoted from #118, not reproduced. They carry #122's caveat that they represent one judge, one victim model and two attacks rather than a range across judges — the ADR states that caveat rather than presenting the range as settled. Also not verified: who lowered required approvals from 2 to 1, and when. The change happened between 2026-08-13 (observed on #144's merge box) and 2026-08-21, outside this decision; the ADR names the review-depth cost but not the author. Finally, the 2026-08-03 merge-past-a-live-review and the 2026-08-13 lost verdict are taken from #154's account and
pr-gate.sh's header, not independently reconstructed from logs.Security implications
Adds one markdown file; no change to exposure. What it records is security-relevant, and the ADR states both accepted exposures plainly: until enforcement lands, the only mechanical barrier between an unreviewed change and
launchpadis one human approval plus the push restriction, and once checks are required they will be bypassable by five admin accounts. The prohibition on model verdicts gating is itself a control — a check turning green on a model's opinion is an agent approving work, whichlaunchpad/AGENTS.mdrule 1 forbids.Escalations
enforce_adminsoff means the gate binds the people least likely to merge. The last twelve merges intolaunchpadwere performed by two accounts, both admins. Accepted deliberately by @tucktuck101, escalated here so the trade is visible to a reviewer rather than buried in Consequences.