fix(742): inherit an h10 verdict only from an allow-listed reviewer #850
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: timothy/ersatztv#850
Reference in New Issue
Block a user
fixes #742
What shipped
review-verdict.ymldecided whether an existingreview-verdict/h10was a verdict worthinheriting by testing
.creator.login != null. That is satisfied by any account's credential.RENOVATE_TOKENis awrite:repositoryPAT of therenovatebot; unlike #697's registrycredential it cannot be scoped off status-write (Renovate needs repo write to push branches and
open PRs), and secrets are a per-repo store any workflow can reference — so a status posted with it
read back
creator: renovate, non-null, and was inherited.The test is now membership in
H10_REVIEWERS="timothy".vars:entry or a secret: those aresettable without a PR, i.e. without this gate, while
.gitea/is on the PROTECTED path list so theedit cannot ride a docs-only exemption.
[ = ]over word-split entries — never[[ == ]](RHS becomes a pattern), nevergrep -xF(a newline-bearing creator reads as several patterns).::warning::names the creator whenever the job re-derives over a verdict-shaped status from anoff-list account, so a reviewer left off the list is not discarded silently.
The part worth reviewing:
read_existing_verdictanswers TWO questionsIt is called twice, and the callers want opposite widths. The first draft gated one flag and so
narrowed both — cold review caught it, and the defect was reproduced red before it was fixed.
ex_human(allow-listed)ex_attributable(broad) + triple changedcreator != nullThe two calls must compute a predicate that "changed" is measurable against. Before the split they
were identical, so anything firing at the second and not the first had arrived in between. Splitting
broke that: a pre-existing row declined for inheritance is still attributable, so the re-read abstained
on the very row the classification had decided to overwrite, left the forgery standing and posted
nothing. Seven tests went red on exactly that. The state/creator/description triple from the first read
is now snapshotted and compared.
Why the raced check stays broad — and not for the reason first written. The first draft said
"narrowing it would leave an exemption success standing over a rejection". Review showed that is not a
checked reason: a real reviewer is on the list by construction. The real reason is the
misconfiguration case — a second human account added and forgotten from the literal — where this is
the last net before a green stands, and the errors are asymmetric (a repair to
pendingisrecoverable; a wrongly-standing
successis not). The cost is stated at the site: any verdict-shapedrow landing inside the write window trips the sticky sentinel and costs that head its exemption.
Verification
Mutation-proven against the shipped file, not a hand-written mutant — each applied to the tree, run,
then reverted:
[ -n "$ex_creator" ]jq→.creator.login == "timothy"ex_humanif falseat the abstainH10_REVIEWERS=tim*/ empty /bobscripts/tests: 968 passed, 2 skipped.test_hook_fire_log.py::test_the_suite_does_not_write_to_the_PRODUCTION_logwent red once during afull run and passes in isolation on both this branch and a pristine
origin/mainworktree —cross-session interference on the shared production log (#809/#822), not this change.
Live verification is post-merge, and that is stated rather than assumed.
pull_request_targetresolves this definition from
main(ci.gate-trigger-base-resolved), so the new behaviour cannot runon its own PR — the same self-test gap
ci.exemption-provenancerecords for route 1. A livecross-account probe would additionally require minting a second Gitea account's credential; the
pre-merge evidence is the executed-behaviour tests above, which run the workflow's real classify shell.
What this does NOT close
An allow-listed collaborator's own write-scoped token can still post a verdict nobody formed;
branch protection binds the context name, not its issuer. And
post-review-verdict.shdoes not checkthat its own account is one the gate accepts — a changed writer credential would deadlock PRs rather
than fail loudly. Filed as #845, deliberately not bundled: that script is on the merge-consent path.
Docs
docs/ci-cd.md(Review-verdict gate),ci.exemption-provenance,ci.actions-credential-scoping,release.verdict-status-check, andci.gate-trigger-base-resolved— the last found by sweeping for theold wording rather than only the files this change touched; it stated in both its rule and its body
that a
RENOVATE_TOKENstatus "carries a real creator and IS inherited". Catalog regenerated.Review history — 11 rounds, and what they changed
Recorded because several rounds changed the DESIGN, not just the diff.
ex_attributable(broad) fromex_human(allow-listed)failureshort-circuit's attributability was unpinned; three pre-existing post-write BLOCKERs → #849pendingis re-derived anyway; no retry trigger exists) → withdrew themTwo mechanisms were withdrawn rather than patched a third time, per this repo's own rule. Both withdrawals are recorded in
ci.exemption-provenanceso the next reader doesn't re-attempt them the same way.Follow-ups filed: #845 (
post-review-verdict.shdoesn't check its own account is allow-listed) and #849 (post-write verification: three routes leaving an exemptionsuccessover a humanfailure, plus the retarget fence's post-POST gap).Round-4 cold review found the annotation's `ex_human != yes` conjunct was the diff's only GREEN mutation, and that the assertion captioned as pinning it could not fail: both parametrizations of that test exit at a short-circuit two guards earlier and never reach the block at all. A caption claiming a proof is worse than no assertion. WITHDRAWN rather than patched a third time. That mechanism produced three defects in three rounds — a message reading "from 'timothy', which is NOT in H10_REVIEWERS ('timothy')", an unescaped API value interpolated into a workflow command, and now a vacuous pin — and it duplicated all three values from the plain log line one line above it. Its only delta was a severity whose rendering could not be probed. Deleting it removes the green mutation, ~28 comment lines, the vacuous assertion and a docs paragraph that had promoted it to a contract. The log line keeps the whole diagnostic and gains the remediation sentence, and its three parts are now asserted on a path the job always reaches. Both halves mutation-proven: dropping the remediation reddens, and so does dropping the allow-list VALUE — an earlier assertion tested for the WORD `H10_REVIEWERS`, which the remediation sentence satisfies on its own, so that deletion had stayed green. Also: the comment justifying POSIX `[ = ]` claimed `[[ == ]]` makes its RHS a pattern. Probed on bash 5.3 — with a QUOTED RHS it does not; only an unquoted one globs. The invariant is the quoting, and it is now stated as that, with the untestability claim (which rested on the false premise) dropped. Round-by-round narrative trimmed out of the code comments per docs.no-session-narrative; the rules those blocks carry stay, the chronology moves to the PR. fixes #742 Decisions-Edit: yesThird sweep, and the third time the same mistake: I grepped for the exact wording I had written and missed the same claim in PARAPHRASE at sites I never opened. Cross-family review enumerated them. This closes both families by concept. Family 1 — "the retarget fence resolved the race". Corrected at every site, not just the two I remembered: the fence record's own rule and its "handoff, not a stall" induction (which is a property of a run that ABSTAINS and says nothing about one that already passed its final count and then posts), ci.exemption-provenance's "Resolved 2026-08-03" heading, and docs/ci-cd.md's "That residual is now fenced". It NARROWS: a retarget between the final pre-write count and the POST leaves a PERMANENT forged green, because the successor consumes the edited event and exits before the stale run posts last. Family 2 — "a pending write is harmless". Retracted in the workflow's untrusted-count rationale ("for no safety gain"), in the same phrase inside the fence record's rule, and in docs/ci-cd.md twice, including the absolute "a human BLOCKED can never be silently turned green". A generic pending blocks the merge IMMEDIATELY, which is what makes letting it through the right trade — but it is not inert: it masks a rejection landing in its own write window, post-write verification does not run for it, and a later run re-derives it into an exemption success with that rejection below the new mark. Letting it through is still right; the trade is "immediate block, later re-derivation risk", not "free". Every retraction names #849, which carries the behaviour work. No behaviour change: the classify step's executable content is identical across all three docs commits (323 lines, comments and blanks stripped, compared via the YAML-extracted step rather than line ranges). scripts/tests: 1012 passed, 2 skipped. fixes #742 Decisions-Edit: yesReview-verdict: MERGEABLE @
586577c11 cold review rounds, 6 cross-family; final verdict clean with 9 mutations all reddening. Residuals deferred to #845 and #849.