Skip to content

ci: add non-cancellable Main Ratchet Audit for main (#7511) - #7971

Merged
NicholasRBowers merged 1 commit into
mainfrom
feat/main-ratchet-audit
Sep 3, 2026
Merged

ci: add non-cancellable Main Ratchet Audit for main (#7511)#7971
NicholasRBowers merged 1 commit into
mainfrom
feat/main-ratchet-audit

Conversation

@bolichen97

@bolichen97 bolichen97 commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Closes #7511.

Problem

main's CI does not reliably render a verdict for the ratchet / ceiling / baseline gates, for two independent reasons:

  1. Supersession. ci.yml's concurrency group is keyed on the ref and cancel-in-progress is 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 mostly cancelled, never failure. A cancelled run is not a red X: main looks green-ish (many successes, zero failures) while drift accumulates unseen.
  2. Surface gating. Since perf(ci): gate four single-surface jobs and stop the code-review pile-up #7337 the lint lanes are surface-gated (backend-lint on only_frontend != 'true', frontend-lint on only_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)

  • Triggers on push to main and workflow_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).
  • concurrency keyed on github.sha (not the ref) with cancel-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.
  • Two gate jobs run unconditionally (both surfaces, no only_backend/only_frontend guard): 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.
  • A report job reconciles a single ratchet-audit-labeled tracking issue, so drift is visible on main and attributed to the push that caused it.

scripts/ratchet_scope.py

  • New RATCHET_SCOPE_WHOLE_TREE opt-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 to main that diff is empty — the checkout leaves HEAD, main and origin/main all 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 to main can answer.

CONTRIBUTING.md and docs/ci/ci-and-reviews.md

  • The contributor-facing half: what a ratchet gate is, why one can go red for something outside your diff, and the low-friction path — land the correction as its own small PR rather than raising the ceiling inside an unrelated change. ci-and-reviews.md carries the mechanism next to the rest of the CI map.

Expect the first run on main to be red, with an actionable list

Whole-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 a ci: change.

The eslint ceiling is read, not transcribed

The first revision transcribed --max-warnings 597 into the new workflow. main then 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 tree ci.yml pins at zero — #7511's own failure mode, reproduced inside the lane added to prevent it. The value is now grepped out of ci.yml at run time (the same extraction the prepare-pr profile uses, except that an unreadable ceiling fails closed), so ci.yml stays the one numeric source. test_eslint_warning_ceiling.py gains test_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:

  • Option 1 (full serialization): dropping cancel-in-progress for main entirely, or adding a per-SHA push group / merge queue on ci.yml itself. Full serialization raises the runner bill in proportion to merge rate, a budget/branch-protection call.
  • Option 2 (another full run): test-durations.yml already runs the full backend pytest on main, but with || true swallowing the verdict, and it cannot see the eslint ceiling. Adding another full run would re-pay those minutes for no new coverage.

Testing

  • The reporter-step harness resolves its shell instead of naming it. test_main_ratchet_audit.py pins the report step by executing it, and it spawned bash by bare name. On the Windows runner that lands on C:\Windows\System32\bash.exe — the WSL launcher, which CreateProcess searches 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 stubbed gh'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:
    • The shell is resolved once to an absolute path, so PATH or CreateProcess search order can never substitute a different program for the one the precondition approved.
    • The shell precondition moves onto the four shell-executing classes and returns None on Windows, matching the nine sibling modules that execute a workflow run: block against a stubbed gh (the step is runs-on: ubuntu-latest, and the extensionless #! stub needs an execute bit os.chmod cannot grant there). The two workflow-parity classes read only YAML, so they stay measured on Windows.
    • _run asserts the call log is non-empty: the step's first action is always gh 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, and TestABrokenShellIsNotAQuietReporter pins it on every platform by supplying such a shell directly. Capture also decodes with errors="replace" so a non-UTF-8 reply is reported rather than raised over.
  • test/test_ratchet_scope.py::TestWholeTreeOverride pins 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 to git diff as a revision, and that the audit lane opts in.
  • test/test_eslint_warning_ceiling.py::test_no_other_workflow_transcribes_the_ceiling closes the second-copy class for every workflow, not just this one.
  • test/test_workflow_checkout_credentials.py (the repo-wide ratchet that caught the missing persist-credentials: false on the frontend job) is green.
  • The four non-black backend gates (subprocess-encoding, agent-SDK-boundary, sync-IO-in-async, lockdown-before-publish) verified green in whole-tree mode as well as diff-scoped; black reports the 11 pre-existing offenders described above, which is the drift this lane exists to surface. The ceiling extraction verified to read 0 from ci.yml and to fail closed on an unreadable one; every run: block passes bash -n; the workflow parses as YAML.
  • Full gate sweep green: isort, flake8, mypy --platform linux, the scripts/check_*.py family, 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).

@bolichen97
bolichen97 requested a review from a team as a code owner September 2, 2026 18:57
@bolichen97
bolichen97 requested a review from CrysisDeu September 2, 2026 18:57
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 2, 2026
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of c4299f90161e9f73e1475735ab34e4bfdcd30f2c — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

  • The reporter's ~200-line run: block is the design's one fragile spot — the test harness must extract it from YAML, execute it via a resolved shell, and defend against wrong-binary/UTF-16LE hazards it created; a scripts/ Python reporter invoked by the step would be directly testable and delete that whole harness class.

[DESIGN-REVIEWED] c4299f9

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed c4299f90161e9f73e1475735ab34e4bfdcd30f2c — this comment is updated in place on each push.

Review details

No blocking findings — one advisory.

FINDING — .github/workflows/main-ratchet-audit.yml:404 — the all-green self-heal branch runs gh issue comment "$EXISTING" and gh issue close "$EXISTING" --reason completed unguarded under set -euo pipefail, so a transient GitHub API failure (5xx/rate-limit) on either call trips set -e and reds the report job on a commit whose ratchet gates all passed — a false drift signal on healthy main (and if the comment succeeds but the close fails, the issue keeps a "Closing." note while staying open), unlike the convergence-loop mutations (|| true, lines 300/302) and the create branch (explicit rc handling); the drift branch's unguarded comment (line 350) is exempt because it exit 1s regardless, but the green branch is meant to succeed → Fix: append || true to the green-path gh issue comment/gh issue close so a bookkeeping hiccup cannot manufacture a red on a green commit.

[OPUS-REVIEWED] c4299f9

Verdict parsed from the review's SHA-scoped output markers for commit c4299f90161e9f73e1475735ab34e4bfdcd30f2c.

False positive or not applicable? A repository writer can comment:
/ai-review override fable c4299f90161e9f73e1475735ab34e4bfdcd30f2c: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of c4299f90161e9f73e1475735ab34e4bfdcd30f2c — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All checks run. The premises verify against the repo: ci.yml genuinely surface-gates its lint lanes (only_backend/only_frontend at ci.yml:556, 1381, 1685, etc.) and keys concurrency on the ref; changed_paths() already had a "None = whole tree" contract the new override reuses rather than duplicating; the two existing issue-creating workflows (add-contributor.yml, fix-loop-analysis.yml) do find-or-create only, with no open/comment/close lifecycle, so the reporter is not a second spelling of either.

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 ships

Intent: give every push to main a reliable ratchet/ceiling verdict so drift stops surfacing on unrelated PRs — an ADDITION answering a reported defect.

  1. Every main push gets a non-cancellable ratchet verdict — justified (reported defect ci: main's runs are cancelled before the ratchet gates report, so drift surfaces on unrelated PRs #7511)
  2. eslint ceiling now measured even on backend-only merges — justified (surface-gate hole, declared)
  3. Auto-managed "Main ratchet drift" tracking issue + ratchet-audit label — justified; consumed by CONTRIBUTING's step 2
  4. RATCHET_SCOPE_WHOLE_TREE opt-in — justified; reuses the existing None-scope contract; 1 consumer, singular form
  5. Ceiling read from ci.yml at run time; second copies test-banned — justified (drift already happened once, refactor(frontend): burn the eslint warning ceiling to zero #7569)
  6. Gate-parity test with two recorded exclusion registries — justified (closes the silent-omission class)
  7. CONTRIBUTING ratchet section — mandated same-commit doc (AGENTS.md)
  8. ci-and-reviews.md entry — mandated same-commit doc
  9. workflow_dispatch trigger, barred from touching the issue — declared
  10. 11 unformatted files deliberately left red for a follow-up PR — declared

Watch

  • Four whole-tree gates sharing the same evicted-verdict cause are counted and deferred in _DEFERRED_WHOLE_TREE_GATES (test/test_main_ratchet_audit.py) — accepted-and-deferred by the author, but it is real unfixed-sibling scope someone must land.
  • The first run on main is a deliberate red; the follow-up PR fixing the 11 files should land promptly or the tracking issue becomes permanent wallpaper.

Subtractions

  • Shrink main-ratchet-audit.yml's 52-line header comment (lines 9–60) to a pointer: it restates the rationale now carried nearly verbatim by docs/ci/ci-and-reviews.md, making three prose copies (header, ci-and-reviews.md, CONTRIBUTING.md) that must drift together.

[FIRST-PRINCIPLES-REVIEWED] c4299f9

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of c4299f90161e9f73e1475735ab34e4bfdcd30f2c and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- .github/workflows/main-ratchet-audit.yml:77 -- "main-ratchet-audit-${{ github.sha }}" lets repeated manual dispatches evict a pending push audit, while manual runs skip reporting -> Fix: include github.event_name in the group key. (origin: validation)
[GPT-REVIEWED] c4299f9

False positive or not applicable? A repository writer can comment:
/ai-review override gpt c4299f90161e9f73e1475735ab34e4bfdcd30f2c: <one-sentence reason>

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 2, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor

The audit lane's eslint ceiling is stale against current main, and not through any fault of this head.

.github/workflows/main-ratchet-audit.yml:168 runs npx eslint src/ --max-warnings 597. Main's real gate is now --max-warnings 0, a single occurrence at .github/workflows/ci.yml:1501. It was burned to zero by c71ec9d04 ("refactor(frontend): burn the eslint warning ceiling to zero", #7569) at 2026-09-03T03:33:04Z, roughly 8.6 hours after this PR's head 4657bbdd was pushed at 2026-09-02T18:54:51Z. The value was correct when it was written, so this drift was not avoidable from your side.

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 ci.yml reds. That is #7511's own failure mode, a false green on main that surfaces later on an unrelated PR, reproduced inside the lane added to prevent it. That is the reason this seemed worth a comment rather than a shrug.

Nothing on main catches the second copy. test/test_eslint_warning_ceiling.py reads ceilings from one file only (_CI at line 26 is .github/workflows/ci.yml), and its prose scan (_PROSE at lines 29-33) covers only docs/, website/docs/, AGENTS.md and website/AGENTS.md, with no path under .github/workflows/. The transcription test at line 77 is the one that would otherwise flag a duplicated value, and it returns early at lines 94-95 when the ci.yml value is 0, on the stated reasoning that a zero ceiling has nothing to go stale. So all three tests pass with 597 sitting in the audit workflow. The early return is the mechanism worth naming: burning ci.yml to zero is precisely what disarmed the check that would otherwise have caught this.

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.

@bolichen97

bolichen97 commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator Author

span=cc8521780a13

  • Census step runs whole test modules, so an unrelated failure reads as ratchet drift (main-ratchet-audit.yml:171) — rebutted; the remedy would re-open the silent-omission class this lane exists to close

"test/test_security_posture.py test/test_config_baseline.py" runs unrelated functional tests, whose failure is reported as ratchet drift -> Fix: select only the census and committed-baseline parity node IDs.

The mechanism is accurate: the step names two whole modules, and test_security_posture.py carries far more than the redactor census, so a functional failure there opens an issue titled "Main ratchet drift detected". I am keeping the whole-module selection, because the proposed remedy costs more than the imprecision it removes.

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 (`ratchet-gates` (backend black/subprocess/agent-sdk/sync-io/lockdown + census & config-baseline): **failure**), and it links the run, whose log names the failing test. A maintainer is one click from the actual cause, and the artifact never asserts which ratchet moved.

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 needs.<job>.result to a dependent job, so step-level outcomes have to be plumbed through job outputs or an artifact first. It is recorded as deferred rather than done here, and it is strictly additive to the reporter when it lands; it does not argue for narrowing what the lane measures in the meantime.

@bolichen97

bolichen97 commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator Author

Engineering notes — two GPT 5.6 findings from an earlier head

The GPT lane re-ran on head 4657bbdd and rewrote its comment in place, so the two blocking findings these notes answered (span 43cdb7a7a10f) no longer resolve to anything in the ledger. The reasoning is kept here as a plain comment rather than as a disposition record, because a record claiming a span the lane no longer prints is itself a readiness violation. Both fixes are in the pushed commit; the lane's one current finding is answered in its own record.

1. Diff-scoped green run closes unresolved drift

Push A leaves an unformatted file -> push B scopes black to B's files -> report closes the issue while A's violation remains.
Fix: Remove automatic closure; leave the issue open for whole-tree verification.

The mechanism is real, and worse than described. I verified it and the premise understates the damage: on a push to main the scope is not "push B's files", it is empty. actions/checkout leaves HEAD, main and origin/main all at the pushed commit, so ratchet_scope.changed_paths() resolves origin/main...HEAD to an empty path set with exit 0 — no attempt fails, nothing marks the answer as unusable. Four of the five backend gates (black, subprocess-encoding, agent-SDK boundary, sync-IO-in-async) then filter their violations against that empty set, so they were guaranteed-green no-ops in this lane, and only black's unscoped graduated half measured anything at all. main is linear (single-parent squash merges), so is_merge is false and there is no shape in which this resolves differently.

Fixed by removing the cause rather than the symptom: scripts/ratchet_scope.py gains a RATCHET_SCOPE_WHOLE_TREE opt-in and the audit job sets it, so the lane judges the integrated tree — the only question a push to main can answer, and the one the workflow claims to answer. It lands in the shared resolver, which is where the module's own docstring says a scope fix belongs ("a scope fix applied to one private copy and not the others would make the same added line red under one gate and green under another"), so all four gates are corrected in one pass rather than the one this finding named. test/test_ratchet_scope.py::TestWholeTreeOverride pins 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, and that this workflow opts in.

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 cancelled or skipped lane) was already handled: anything other than an explicit success is treated as drift and cannot reach the closing branch.

Worth naming as evidence that the fix bites: whole-tree scope immediately finds 11 files that are unformatted and absent from .github/black-baseline.txt, which no pull request's diff-scoped gate can see. That is the drift class #7511 is about. Per the contributor path this PR documents, they are not reformatted here — that correction belongs in its own small PR, not buried in a ci: change.

2. Per-SHA reporters can create duplicate tracking issues

Two failing audits list concurrently -> both see no issue -> both create one; later runs select only one, leaving stale state.
Fix: Add constant job-level concurrency around the report reconciliation.

The race is real: the concurrency group is keyed on github.sha deliberately, so two drifting pushes can reach the reporter at once, both find no open issue and both create one. The consequence is the "stale state" half — the next green push closes only the one it selected and the other stays open forever as a false live drift record.

I rejected the proposed remedy, because a constant group around report trades this for a strictly worse failure in the same workflow. GitHub keeps only one pending run per concurrency group: that is the platform behaviour this entire PR exists to work around, quoted in the workflow header and in ci.yml:14-19. Serialising the reporters therefore means a third push evicts a queued reporter, and an evicted reporter renders no verdict at all — losing the drift record outright rather than duplicating it. Paying a lost verdict to avoid a redundant issue is the wrong trade for a lane whose whole purpose is that the verdict always renders.

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 <!-- main-ratchet-audit-tracking-issue --> marker, because the label alone is not proof of authorship and this code now closes what it selects, so it must never reach a human's issue that merely carries the label; and the marker was previously write-only, so giving it a reader also closes the First Principles lane's separate observation that it had zero consumers, without deleting the only thing that can distinguish the workflow's own issues from anyone else's.

3. Transcribed --max-warnings 597 permits warnings canonical CI rejects

Also superseded: the GPT lane raised this at head 4657bbdd and no longer prints it, since the fix is in the pushed commit. Kept for the record.

"--max-warnings 597" permits warnings that canonical CI's zero ceiling rejects, hiding main drift -> Fix: extract the ceiling from ci.yml.

Legitimate, and already realised rather than hypothetical: main burned the ceiling down to 0 in #7569 while this PR sat open, so the transcribed 597 was stale on arrival. An anti-drift lane running a looser ceiling than the gate it audits is worse than no lane at all — it reports green on frontend drift that a pull request's own gate would red, which is the precise false all-clear the workflow exists to close. The asymmetry is what makes a number the dangerous thing to copy: a renamed gate script fails loud here, whereas a stale ceiling fails silently permissive.

Fixed with the remedy the finding names. The step now reads the ceiling out of ci.yml at run time (grep -oE 'npx eslint src/ --max-warnings [0-9]+', first match) and there is no numeric literal left in this workflow. It fails closed: an unreadable ceiling emits ::error::could not read the eslint ceiling out of .github/workflows/ci.yml; refusing to guess one and exits 1, because a gate that cannot find its own ceiling must not pass. That is the same extraction the prepare-pr profile already performs, so ci.yml stays the one numeric source.

Closing the class rather than the instance, since a second transcription would reintroduce it silently: test/test_eslint_warning_ceiling.py::test_no_other_workflow_transcribes_the_ceiling scans every .github/workflows/*.yml except ci.yml for a --max-warnings <digits> literal and fails on any hit. It is checked at every ceiling value including zero — deliberately, because the pre-existing prose test returns early when the ceiling is 0, and that early return is exactly what disarmed the check that would otherwise have caught the 597. @chenmingwei23 independently reported the same staleness on this PR and diagnosed that mechanism; their framing is what led to auditing the scope of the other four gates in this lane.

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • Design Review 🟡 CONCERNS — "an anti-drift gate built on an un-gated copy" — fixed

the gate commands and --max-warnings 597 are "copied byte-for-byte from ci.yml… If ci.yml changes a gate command or the eslint --max-warnings ceiling, mirror it here," with the duplication "guarded by comment only."
on the first ratchet-down that forgets the mirror, the audit reports main green at 597 while PRs fail at the tighter ceiling, and the authoritative-looking green audit now actively argues against the PR author who inherited the red.

Accepted in full, and the lane was right about the timeline as well as the mechanism: main burned the ceiling to zero in #7569 while this PR sat open, so the predicted drift had already happened before the review round closed. A copy is not prose that goes stale — it is a second enforced ceiling that keeps granting the old budget, which is #7511's own failure mode reproduced inside the lane added to prevent it.

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 ci.yml at run time, the same extraction prepare-pr/profiles/kirocrew.json already uses, with one difference — an unreadable ceiling fails closed here, because a gate that cannot find its own ceiling must not pass. The two "mirror it here" comments are gone with the value.

On the suggestion's other half, gating the command list by grepping ci.yml: I did not take it. A grep asserting one workflow's step list against another's is a brittle textual coupling that reds on formatting, and it buys less than the extraction does — a renamed gate script is a loud failure in this lane (the step exits non-zero on a missing file) whereas a stale number is silently permissive, which is the asymmetry that made the ceiling the dangerous copy. The remaining duplication is command names, and it now fails toward red.

test/test_eslint_warning_ceiling.py gains test_no_other_workflow_transcribes_the_ceiling, which converts the comment-only contract into a verdict for every workflow rather than just this one. It is checked at every value, including zero: the early return that spares prose quoting the real invocation cannot cover a second gate, and that early return is precisely what disarmed the existing tests here.

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • First Principles 🟡 CONCERNS — the second hardcoded 597, and the dead dedup marker — both fixed

test/test_eslint_warning_ceiling.py:1 pins "The frontend eslint warning ceiling has ONE numeric source… The value belongs only in .github/workflows/ci.yml." The PR's "guarded by comment only… out of scope" re-opens exactly the transcription failure that test exists to prevent
Replace the literal 597 in main-ratchet-audit.yml:168 with the repo's existing extract-from-ci.yml spelling — prepare-pr/profiles/kirocrew.json:46 already greps the ceiling out of ci.yml at runtime
Delete MARKER (main-ratchet-audit.yml:194) and the comment claiming it "makes every run converge on ONE issue": dedup is label-based (the :222 comment says so), and grep main-ratchet-audit-tracking-issue counts 1 write site, 0 readers.

Both accepted; the pointer to the existing spelling was the useful part and I took it. The ceiling is now grepped out of ci.yml at run time exactly as prepare-pr/profiles/kirocrew.json does it, with the empty case failing closed rather than silently skipping, and both "mirror it here" comments are deleted with the number. The lane's diagnosis of why nothing caught it is also correct and is now closed at that level: the transcription test returned early once ci.yml read 0, so burning the ceiling down disarmed the check that would have caught the copy. test_no_other_workflow_transcribes_the_ceiling scans every workflow at every value, including zero, because a second gate is never the accurate prose the early return exists to permit.

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 ratchet-audit label is not proof of authorship — anyone can add it to their own issue. So the marker gained a reader instead of being deleted: selection filters the label-scoped list by it, and its comment now states that authorship job rather than the dedup claim it never performed. One write site, one read site, and the "0 readers" observation no longer holds.

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.

@bolichen97

Copy link
Copy Markdown
Collaborator Author

@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 .github/workflows/ci.yml at run time (the same extraction prepare-pr/profiles/kirocrew.json already uses), with one difference: an unreadable ceiling fails closed here, because a gate that cannot find its own ceiling must not pass. Both "mirror it here" comments are gone with the value. Verified locally that the extraction reads 0 from current ci.yml and yields nothing on an absent line.

Your point about the disarmed check is the one I acted on hardest, because it generalises past this instance: the early return at test_the_ceiling_is_not_transcribed_into_prose lines 94-95 turns on a doc quoting the gate's real invocation being accurate prose, and a second gate is never that — so burning ci.yml to zero silenced the only test that could have flagged the copy. test/test_eslint_warning_ceiling.py now also carries test_no_other_workflow_transcribes_the_ceiling, which scans every workflow under .github/workflows/ other than ci.yml, unconditionally at every value including zero. A future burn-down cannot re-disarm it, and the class is closed for any workflow, not just this one.

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 main the four diff-scoped gates resolve origin/main...HEAD to an empty path set, because the checkout leaves HEAD, main and origin/main all at the pushed commit. They were passing by measuring nothing. The lane now sets RATCHET_SCOPE_WHOLE_TREE so it judges the integrated tree, and the first thing that finds is 11 files unformatted and absent from .github/black-baseline.txt that no PR's diff-scoped gate can see. Those go in their own PR, per the path this one documents.

@bolichen97
bolichen97 force-pushed the feat/main-ratchet-audit branch from 4657bbd to 8714b74 Compare September 3, 2026 12:12
@bolichen97

Copy link
Copy Markdown
Collaborator Author

span=3245efe9044e

  • frontend-ceiling checkout omitted persist-credentials: false (main-ratchet-audit.yml:153) — fixed

the frontend-ceiling checkout omits persist-credentials: false, so actions/checkout's default writes the job's GITHUB_TOKEN (this workflow grants issues: write) into .git/config, and the very next step runs npm ci --no-audit --no-fund, executing dependency lifecycle scripts with that token readable in the workspace; the sibling ratchet-gates job (line 89) and every ci.yml frontend job that does not git fetch set persist-credentials: false, and this job never fetches

Legitimate, and it was the load-bearing half of a red rather than a style note: test/test_workflow_checkout_credentials.py::test_every_checkout_either_opts_out_or_documents_why named this exact checkout and failed on both Backend Tests (3.12, 4) and Backend Tests (Windows) (4). The mechanism is as described — the job's token carries issues: write, the default checkout persists it into .git/config, and the immediately following npm ci runs third-party lifecycle scripts in the same workspace.

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 frontend-ceiling checkout now carries persist-credentials: false, matching the sibling ratchet-gates checkout, and the pinned test passes on both platforms.

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.

@bolichen97
bolichen97 force-pushed the feat/main-ratchet-audit branch from 8714b74 to d22d175 Compare September 3, 2026 12:31
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 3, 2026
@bolichen97
bolichen97 force-pushed the feat/main-ratchet-audit branch from d22d175 to c02d3a4 Compare September 3, 2026 12:40
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 3, 2026
@bolichen97
bolichen97 force-pushed the feat/main-ratchet-audit branch from c02d3a4 to cac1c2e Compare September 3, 2026 12:55
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 3, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • Design Review ✅ PASS — two suggestions on the tracking issue's usefulness — accepted-and-deferred, with the mechanism that makes each more than a wording change

The reporter comments "Still drifting" on every drifting push; at the cited ~1.4min merge gap, a stalled fix (e.g. the known 11-file black drift) accumulates hundreds of identical comments in days. Comment only when the failed-lane set changes since the last comment, and let the red run carry per-push attribution.
The issue bullets name only the failed job ("ratchet-gates: failure"), so distinguishing a new drift class from the known one requires opening run logs; surfacing the failed step names in the bullet would make the tracking issue self-sufficient.

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 needs.<job>.result, so step-level outcomes must first be published as job outputs or an artifact. Until then the bullet deliberately names the lane and not a specific ratchet, so the artifact is imprecise rather than wrong, and the run link carries the detail.

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 ci.yml rather than backend-lint alone, and each gate is either measured, recorded as judging-a-diff-not-a-tree, or named in _DEFERRED_WHOLE_TREE_GATES — so the boundary is a reviewed decision and a gate added tomorrow reds until classified.

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • First Principles ✅ PASS — the four counted siblings left unmeasured on main — accepted-and-deferred, deliberately named rather than silently omitted

Four counted siblings of the evicted-verdict cause stay unmeasured on main (check_feature_map, check_builtin_skill_scope, check_loop_bound_locks, check_testpaths_coverage — all confirmed in ci.yml). Recorded in _DEFERRED_WHOLE_TREE_GATES with a promotion path.

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 ci.yml's backend-lint, which missed the repo's dominant shape — one gate per standalone job — so a future gate added that way would have been invisible to a test whose docstring claimed to catch exactly that. Nine gate scripts were outside the pin entirely.

The pin now reads every job in ci.yml, and each gate script lands in exactly one of three sets: measured by ratchet-gates; _PR_ONLY_BY_CONSTRUCTION (it judges a diff against a base ref, diffs the base branch's own file, or consumes a coverage artifact this lane never produces, so there is no whole-tree question to ask on main); or _DEFERRED_WHOLE_TREE_GATES, the four named above. An unclassified gate reds the test, so a gate added tomorrow forces a decision in a reviewed diff instead of quietly never running.

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 ci: change whose stated subject is the ratchet/ceiling/baseline set would make the diff harder to review than the gates are to add. The docstring claim was narrowed to what the pin actually covers — scripts/check_*.py invocations — and the two pytest-side ratchets are documented as an original selection with no counterpart in ci.yml to diff against, since a source-heuristic pin was measured (five modules match for the two that are ratchets) and rejected.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 3, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 3, 2026
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.
@bolichen97
bolichen97 force-pushed the feat/main-ratchet-audit branch from cac1c2e to c4299f9 Compare September 3, 2026 19:15
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 3, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

Engineering notes — the two advisory findings on c4299f90161e9f73e1475735ab34e4bfdcd30f2c

Both blocking lanes report no blocking findings on this head (GPT 5.6 ✅, Opus 4.8 ✅, First Principles PASS, Design PASS), so neither item below is a disposition — recording the reasoning so the next reader does not have to re-derive it.

Opus's advisory (green-path gh issue comment / gh issue close unguarded under set -euo pipefail). Verified against the diff: those two calls are indeed the only issue mutations without || true or explicit rc handling. Not taken, because the suggested || true makes the worse half of the outcome Opus itself names silent: if the comment lands and the close does not, the tracking issue keeps a "Closing." note while staying open, and with || true nothing anywhere says so. Failing loud is the honest answer — the report job is named "Render ratchet verdict" and is separate from the two gate jobs, so its red reads as "the reporter could not finish", not as "main drifted"; the gate verdicts stay legible on their own check runs, and a maintainer can re-dispatch. The asymmetry with the convergence loop is deliberate for the same reason: closing a redundant duplicate is optional bookkeeping that can never lose the last record, whereas closing the canonical issue is the whole self-heal.

GPT's advisory (main-ratchet-audit-${{ github.sha }} shares a group between a push and a manual dispatch of the same SHA). Real, and narrower than it reads: it needs a workflow_dispatch on main while that exact SHA's push audit is still pending, and the cost is that the dispatch run — which skips report by design — replaces a queued push run that would have written the verdict. The push audit is re-obtainable by dispatching again, so the failure mode is a delayed verdict rather than a lost one, and the per-SHA key is what buys the property this lane exists for (no push can supersede an earlier push's audit). Deliberately left as one key rather than two, so that "one audit per commit" stays a single fact about the group name; if manual dispatch turns out to be used often enough on main for the race to matter, adding github.event_name is a one-line follow-up that does not change anything else here.

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Sep 3, 2026
@NicholasRBowers
NicholasRBowers enabled auto-merge (squash) September 3, 2026 23:29

@NicholasRBowers NicholasRBowers left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@NicholasRBowers
NicholasRBowers merged commit 6e6c47a into main Sep 3, 2026
67 of 74 checks passed
@NicholasRBowers
NicholasRBowers deleted the feat/main-ratchet-audit branch September 3, 2026 23:30
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ci: main's runs are cancelled before the ratchet gates report, so drift surfaces on unrelated PRs

3 participants