ci: add non-cancellable Main Ratchet Audit for main (#7511) - #7971
Conversation
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS A real measurement gap on main, closed by a reversible, well-falsified audit lane whose failure modes are pinned by executing the actual step. Suggestions
[DESIGN-REVIEWED] c4299f9 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsNo blocking findings — one advisory. FINDING — .github/workflows/main-ratchet-audit.yml:404 — the all-green self-heal branch runs [OPUS-REVIEWED] c4299f9 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All checks run. The premises verify against the repo: ci.yml genuinely surface-gates its lint lanes ( First-Principles-Verdict: PASS A reported defect (#7511, main renders no ratchet verdict), and every item traces to it, names its alternatives, and records what it defers. What this change shipsIntent: give every push to main a reliable ratchet/ceiling verdict so drift stops surfacing on unrelated PRs — an ADDITION answering a reported defect.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] c4299f9 |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsFINDING -- .github/workflows/main-ratchet-audit.yml:77 -- False positive or not applicable? A repository writer can comment: |
|
The audit lane's eslint ceiling is stale against current main, and not through any fault of this head.
The consequence in one sentence: the audit lane whose entire purpose is rendering the ceiling verdict on main would grant 597 warnings of slack against a tree that main pins at zero. It is latent today, because the tree measures zero warnings, which is what made the burn-down possible in the first place. It starts mattering the first time a warning lands on main: the audit lane reports green while Nothing on main catches the second copy. A rebase already looks to be on this PR's path, with six red lanes (GPT 5.6 Review, PR Hygiene, Backend Tests (Windows) (4), Backend Tests (3.12, 4), Coverage Gate, PR Readiness) and PR Hygiene wanting one commit against the current four, so correcting the number should cost no extra push. The mirror-instruction comments at lines 40 and 161 now point at a value that has moved, which is the same single-source concern the First Principles lane raised. Read-only pass against main at the time of writing. The branch and the file were not touched. |
|
span=cc8521780a13
The mechanism is accurate: the step names two whole modules, and A node-ID list is a transcription of test identities into the workflow, and its failure mode is not symmetrical with the one it fixes. A renamed node fails loud (pytest errors on an unknown node ID), which is fine. But a new ratchet counter added to either module — a third baseline census, another committed-baseline parity check — is simply not selected, runs nowhere on main's integrated tree, and nothing goes red. That is precisely the silent-omission class this PR spent its other four changes closing: the empty diff scope, the transcribed ceiling, the unmirrored gate, the skipped gate step. Whole-module selection is the one form that cannot acquire that hole, since a ratchet added next to an existing one is picked up by construction. The imprecision that remains is confined to the issue TITLE. The body's failed-lane bullet already names the lane rather than claiming a specific ratchet ( I do accept the concern underneath, which the Design lane raised independently as "the issue bullets name only the failed job": the tracking issue would be more useful if it named the failed STEPS. That is not a wording change, though — Actions exposes only |
Engineering notes — two GPT 5.6 findings from an earlier headThe GPT lane re-ran on head 1. Diff-scoped green run closes unresolved drift
The mechanism is real, and worse than described. I verified it and the premise understates the damage: on a push to Fixed by removing the cause rather than the symptom: I rejected the proposed remedy. "Remove automatic closure; leave the issue open" treats a green verdict as untrustworthy forever, which converts the tracking issue into a permanent unactionable artifact and deletes the self-heal that makes one durable issue workable instead of a growing pile. With whole-tree scope a green run genuinely means the integrated tree is clean, so closing on it is now correct — and the residual case the rationale is really about (a Worth naming as evidence that the fix bites: whole-tree scope immediately finds 11 files that are unformatted and absent from 2. Per-SHA reporters can create duplicate tracking issues
The race is real: the concurrency group is keyed on I rejected the proposed remedy, because a constant group around Fixed by converging instead. The reporter now lists every open issue it owns, treats the lowest-numbered as canonical, and closes the rest as duplicates with a pointer to it. A race then costs one redundant issue for one push and self-heals on the very next run, with no window in which a verdict can be lost. Two details make that safe rather than destructive: selection is filtered by the hidden 3. Transcribed
|
Accepted in full, and the lane was right about the timeline as well as the mechanism: Fixed, and closer to the suggestion's shape than to a refactor. The audit no longer carries a number: it greps the ceiling out of On the suggestion's other half, gating the command list by grepping
|
Both accepted; the pointer to the existing spelling was the useful part and I took it. The ceiling is now grepped out of On the marker I reached the opposite conclusion about the remedy while agreeing with the finding. It was write-only, and the comment claiming it drove dedup was wrong. But deleting it removes the only thing that can tell this workflow's own issues from anyone else's, and the reporter now needs exactly that: closing GPT's duplicate-issue race made the reconciliation close what it selects, and the For completeness on the inventory: item 6 (the second numeric copy) is withdrawn, item 9 (the marker) is now load-bearing, and the "declared but deferred" framing that covered both is gone from the PR description. |
|
@chenmingwei23 — thank you, this was exactly right and it saved the lane from shipping the bug it was written to prevent. Fixed in the rebase, and the mechanism you named is now what the fix is designed around. You called the important part: the value was correct when written, so the drift was not avoidable from my side — which is the whole argument for not transcribing it at all. So the audit no longer carries a number. It greps the ceiling out of Your point about the disarmed check is the one I acted on hardest, because it generalises past this instance: the early return at On the rebase: you were right that it cost no extra push. It also turned up a second and larger instance of the same false-green shape, which your framing is what made me go looking for — on a push to |
4657bbd to
8714b74
Compare
|
span=3245efe9044e
Legitimate, and it was the load-bearing half of a red rather than a style note: Fixed with the remedy named. That test offers two branches, opt out or document why the credential is needed, and the opt-out is the correct one here: the job checks out, installs, and runs eslint, and performs no git operation at all, so there is nothing for a persisted credential to serve. The I audited the rest of the added workflow in the same pass rather than only the line cited: it declares exactly two checkouts, and both now opt out, so no third site of this class remains in the file. |
8714b74 to
d22d175
Compare
d22d175 to
c02d3a4
Compare
c02d3a4 to
cac1c2e
Compare
Both are real, and the first is not hypothetical: this PR ships knowingly red on 11 black offenders, so the comment volume the suggestion describes is the expected day-one behaviour, not an edge case. I am recording both as deferred rather than fixing them here, for reasons that are about mechanism rather than effort. Comment throttling. "Only when the failed-lane set changes" needs the reporter to know the last comment's lane set, which means either parsing its own previous comment body (making a rendering detail load-bearing for a control decision) or keeping that state in the issue body and editing it in place. Both are reasonable; both are a state machine, and one landing in the same commit as the supersession guard would be two concurrency-sensitive behaviours arriving untested together. The volume is also self-limiting in the direction that matters: every comment carries a distinct SHA and run link, so the record stays accurate while it is noisy, and it stops the moment the drift is fixed. Failed step names. This is the same concern the GPT lane raised from the other side, and it is the more valuable of the two. It is blocked on a plumbing detail rather than a decision: a dependent job sees only Neither is load-bearing for the verdict this lane exists to render, and both are additive to the reporter when they land. What the round did change in response to the lane's earlier CONCERNS is the parity pin: it now reads every job in |
The lane was right at CONCERNS and the fix is in this head, so this records the boundary rather than arguing with it. The earlier version of the parity pin read only The pin now reads every job in I am deferring those four rather than adding them here, and the reason is scope honesty rather than cost: they are cheap and they genuinely share the evicted-verdict cause, so the right move is a follow-up that promotes them with their own step and a line deleted from that set. Folding four more gates into a |
main's CI does not reliably render a verdict for the ratchet, ceiling and baseline gates. ci.yml's push runs are evicted while pending on a busy main, so a commit's checks end up "cancelled" rather than "failure" and main looks green-ish while drift accumulates; and the lint lanes are surface-gated, so a backend-only merge skips the eslint ceiling entirely. The drift then surfaces on an unrelated pull request, whose CI runs against merge(branch, main), and the cheapest way out for that author is to raise the ceiling -- exactly what a ratchet exists to prevent. Separate measurement from judgment: run only the individually-cheap ratchet gates, unconditionally on both surfaces, in a workflow whose concurrency group is keyed on the SHA so no push can supersede an earlier push's audit. A report job maintains one labelled tracking issue so drift is attributed to the push that caused it. ci.yml's concurrency and full-run budget are left alone; full serialization or a merge queue is a maintainer budget ruling. The eslint ceiling is read out of ci.yml at run time rather than transcribed, keeping the one-numeric-source invariant that test_eslint_warning_ceiling.py pins. CONTRIBUTING.md explains what a ratchet gate is, why one can go red for something outside your diff, and why the remedy is a separate one-line correction rather than raising the ceiling inside an unrelated change.
cac1c2e to
c4299f9
Compare
Engineering notes — the two advisory findings on
|
NicholasRBowers
left a comment
There was a problem hiding this comment.
Tier 1 auto-approve: chore/CI plumbing (7 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: new non-cancellable CI audit workflow + ratchet-scope env override + tests + contributor docs -- CI plumbing only, no product runtime impact.
Closes #7511.
Problem
main's CI does not reliably render a verdict for the ratchet / ceiling / baseline gates, for two independent reasons:ci.yml's concurrency group is keyed on the ref andcancel-in-progressis false for push, but GitHub still evicts the single pending run when a newer push queues. On a busy main (~1.4min median merge gap against a ~19min run) each run is superseded before its slower lanes report, so a commit's check runs end up mostlycancelled, neverfailure. A cancelled run is not a red X: main looks green-ish (many successes, zero failures) while drift accumulates unseen.backend-lintononly_frontend != 'true',frontend-lintononly_backend != 'true'). On a single-surface push to main (e.g. a backend-only merge) the eslint ceiling is skipped, so it is never measured against the integrated tree at all.The consequence is that drift surfaces later on an unrelated pull request (a PR runs against
merge(branch, main)and inherits main's state), and the cheapest way out for that author is to raise the ceiling, exactly what a ratchet exists to prevent.Approach: separate measurement from judgment
A hybrid of the issue's option 3 (corrected to run the gates unconditionally, closing the surface-gate hole) plus the documentation half of option 4.
.github/workflows/main-ratchet-audit.yml(new)mainandworkflow_dispatch(not on PRs, since a PR already runs these gates against its own diff; this lane's value is evaluating them on main's integrated tree).concurrencykeyed ongithub.sha(not the ref) withcancel-in-progress: false, so a later push can never supersede an earlier push's audit: per-SHA serialization scoped only to the cheap ratchet lanes.only_backend/only_frontendguard): the backend baselined gates (black baseline, subprocess-encoding, agent-SDK boundary, sync-IO-in-async, lockdown-before-publish) plus the census and config-baseline pytest ratchets; and the frontend eslint ceiling.reportjob reconciles a singleratchet-audit-labeled tracking issue, so drift is visible on main and attributed to the push that caused it.scripts/ratchet_scope.pyRATCHET_SCOPE_WHOLE_TREEopt-in. Without it this whole workflow measures nothing. Four of the five backend gates scope their verdict to the files the change in front of them touches, and on a push tomainthat diff is empty — the checkout leavesHEAD,mainandorigin/mainall at the pushed commit, so the three-dot fallback succeeds with an empty path set, exit 0, nothing marking the answer as unusable. Every consuming ratchet then filters its violations against that empty set and reports green whatever the tree holds. The audit lane sets the override so it judges the integrated tree, which is the only question a push tomaincan answer.CONTRIBUTING.mdanddocs/ci/ci-and-reviews.mdci-and-reviews.mdcarries the mechanism next to the rest of the CI map.Expect the first run on
mainto be red, with an actionable listWhole-tree scope is not cosmetic: it immediately finds 11 files that are unformatted and not in
.github/black-baseline.txt, which no pull request's diff-scoped gate can see. That is precisely the drift class #7511 is about, and the audit reporting it is the feature working. Per the path this PR documents, those 11 files are not reformatted here — that correction belongs in its own small PR rather than buried in aci:change.The eslint ceiling is read, not transcribed
The first revision transcribed
--max-warnings 597into the new workflow.mainthen burned the ceiling to zero (#7569), which is exactly the drift a second copy causes: the audit lane would have granted 597 warnings of slack against a treeci.ymlpins at zero — #7511's own failure mode, reproduced inside the lane added to prevent it. The value is now grepped out ofci.ymlat run time (the same extraction the prepare-pr profile uses, except that an unreadable ceiling fails closed), soci.ymlstays the one numeric source.test_eslint_warning_ceiling.pygainstest_no_other_workflow_transcribes_the_ceiling, which is checked at every value including zero — the early return that spared prose cannot cover a second gate.What this deliberately does NOT do (open maintainer decision)
Two of the issue's options are CI-budget rulings and were not taken here, so a maintainer can still layer them on top:
cancel-in-progressfor main entirely, or adding a per-SHA push group / merge queue onci.ymlitself. Full serialization raises the runner bill in proportion to merge rate, a budget/branch-protection call.test-durations.ymlalready runs the full backend pytest on main, but with|| trueswallowing the verdict, and it cannot see the eslint ceiling. Adding another full run would re-pay those minutes for no new coverage.Testing
test_main_ratchet_audit.pypins thereportstep by executing it, and it spawnedbashby bare name. On the Windows runner that lands onC:\Windows\System32\bash.exe— the WSL launcher, whichCreateProcesssearches before any PATH entry, so a probe approving Git Bash on PATH does not prevent it. It answers a UTF-16LE "Windows Subsystem for Linux has no installed distributions" on stderr and exits 1, leaving the stubbedgh's call log empty: seven tests failed on a message about the shell, and an eighth (test_a_superseded_drifting_run_does_not_touch_the_issue, whose expectation is an empty call list) passed vacuously. Three changes, each verified locally against a stub that emits the launcher's UTF-16LE reply and exits 1 — reproducing the runner's exact 7-failed / 4-passed split before the fix:CreateProcesssearch order can never substitute a different program for the one the precondition approved.Noneon Windows, matching the nine sibling modules that execute a workflowrun:block against a stubbedgh(the step isruns-on: ubuntu-latest, and the extensionless#!stub needs an execute bitos.chmodcannot grant there). The two workflow-parity classes read only YAML, so they stay measured on Windows._runasserts the call log is non-empty: the step's first action is alwaysgh label create, so an empty log means the shell never ran the step rather than that the reporter chose to do nothing. This is what turns an unusable shell into one loud failure instead of a split verdict, andTestABrokenShellIsNotAQuietReporterpins it on every platform by supplying such a shell directly. Capture also decodes witherrors="replace"so a non-UTF-8 reply is reported rather than raised over.test/test_ratchet_scope.py::TestWholeTreeOverridepins the hazard itself (a push to the base branch resolves to an empty diff, not a clean tree), the override's answer, that it wins over a resolvable diff, that a blank value is not an opt-in, that the label is never handed togit diffas a revision, and that the audit lane opts in.test/test_eslint_warning_ceiling.py::test_no_other_workflow_transcribes_the_ceilingcloses the second-copy class for every workflow, not just this one.test/test_workflow_checkout_credentials.py(the repo-wide ratchet that caught the missingpersist-credentials: falseon the frontend job) is green.0fromci.ymland to fail closed on an unreadable one; everyrun:block passesbash -n; the workflow parses as YAML.mypy --platform linux, thescripts/check_*.pyfamily,docs-lint.sh, and the CI-structure / workflow-shape test families (test_ci_surface_tests,test_ci_failure_annotations,test_local_gate,test_prepare_pr_profiles,test_ai_review_workflows,test_workflow_*,test_security_posture,test_config_baseline).