fix(ci): ratchet the Ally review-consistency guard so it can signal again (PEN-2847) - #1585
fix(ci): ratchet the Ally review-consistency guard so it can signal again (PEN-2847)#1585allyblockcast[bot] wants to merge 1 commit into
Conversation
…gain (PEN-2847) `ally-review-consistency.yml` failed 39 of its last 40 scheduled runs, red essentially continuously for ~39h, with a byte-identical six-violation set on every failing run. The invariants were computed correctly the whole time. What was lost was the ability to *signal*: a seventh violation on a live PR would have moved the run from red to red and changed nothing downstream, so the incident class this guard exists to catch (BLO-19778) was unguarded while the check appeared to be running. The six findings are pinned by four PRs -- #1304, #1316, #1360, #1525 -- last touched between 1 and 20 days ago, none able to merge. The guard audits every open PR forever and has no notion of that, so one abandoned PR carrying a duplicate Ally review converts a continuous auditor into a constant. So violations are now ratcheted against `scripts/ally-review-consistency-baseline.json`. Every PR is still audited and every violation is still printed; only unbaselined ones fail the run. The load-bearing detail is the baseline key. `violationFingerprint` pins the invariant code, PR number, head SHA and the exact set of review IDs named, discarding the prose -- so an entry expires on its own the moment anything real changes: a push gives a new head, a third review gives a new ID set, a different invariant gives a new code, and another PR was never baselined. All four go red. That is a sharper liveness test than scoping the audit to PRs updated within N days, which was the other candidate and was measured against this repo before being rejected: #1525 sits at `mergeable_state: behind`, inside any plausible allow-list, and was updated 2 days before filing, inside any plausible window. It would have stayed in scope and the run would have stayed red -- option 2 does not satisfy the green half of the acceptance criteria on real data. A PR that could actually merge on a bad attestation is one that is *moving*, and a moving PR breaks its own baseline entry; staleness is a proxy for that, the head SHA measures it directly. Fail-closed choices, matching `assertPrListComplete`/`assertHeadSha` one layer up: a malformed or unattributable entry throws rather than being skipped, since a baseline that ignores its own bad rows is the same fail-open shape this script exists to catch; each entry must name its PR and owning issue; and the fingerprint's PR must agree with the entry's `pr` field. Stale entries are deliberately *not* fatal. Baselined PRs get merged, closed and force-pushed as a matter of course, and failing on that would reintroduce the permanent red this change cures, triggered by the guard's own bookkeeping. They emit a `::warning` annotation; pruning one never changes the verdict. Demonstrated against live repo state, both directions: - Green: exit 0 across all 88 open PRs, with the six known findings still printed and attributed to PEN-2847 rather than hidden. - Red on a violation the baseline does not cover: removing the #1304 entry fails the run naming exactly that violation and its fingerprint. - Red on re-awakening: rewriting #1525's entry head to simulate a push stops the entry matching, and both of that PR's violations are re-audited and fail while the now-stale entries only warn. The unit suite goes 86 -> 114 tests, covering fingerprint stability under rewording, fingerprint change under push/escalation/other-PR, every `parseBaseline` rejection, and an assertion that the committed baseline suppresses exactly the measured set and nothing else. The workflow header said the guard fails on every violation, which is no longer true, so it now describes the ratchet instead of leaving a reader with prose the code contradicts. Disposition of #1304/#1316/#1360/#1525 belongs to their authors and is untouched here; the baseline records the findings rather than clearing them. Refs PEN-2847, PEN-2370, BLO-19778, BLO-22916. Signed-off-by: Devops <devops@paperclip.blockcast.net>
1 similar comment
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 00b36ef
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The ratchet is narrowly scoped and preserves the underlying invariant computation while making known, explicitly attributable findings visible rather than silently dropping them.
- Fingerprints bind the invariant, PR, head, and review-ID set, so pushes, new reviews, and new invariant failures are re-audited.
- Baseline parsing fails closed on malformed, duplicated, or unattributable entries, with focused regression coverage.
Recommended Action
- Merge after the repository CI gates pass.
All CI gates green — ready for human mergeThanks for the review. No findings to address (0 critical / 0 important / 0 suggestions), so the head is unchanged at 19 pass / 0 fail / 0 pending. 2 skipped ( Re-verified the baseline is still exactThe baseline pins other PRs' head SHAs, so it can decay between authoring and merge — if any of the four had moved, this would land and the guard would go red again immediately. Re-checked just now; every head still matches its committed entry:
If one of them is pushed to before this merges, that is the ratchet working as designed — the entry stops matching and the violation is re-audited. I'd re-baseline it rather than widen the rule. One caveat worth stating rather than leaving impliedThe scheduled trigger reads the workflow from Incidentally, this PR carries exactly one Ally review at its head — it satisfies the invariant it repairs rather than adding a seventh violation. Human-merge gated ( |
Thinking Path
Linked Issues or Issue Description
What Changed
scripts/check-ally-review-consistency.mjsviolationFingerprint(violation)— reduces a violation tocode:pr:head:reviewIds, discarding the prose, so rewording a message cannot move a finding out from under its baseline entry but a push / escalation / different PR does.parseBaseline(raw)— validates the baseline document and throws on anything malformed, unattributable, duplicated, or whose fingerprint PR disagrees with itsprfield.applyBaseline(violations, entries)— splits live violations intofailing/suppressedand reportsstaleEntries.main()now fails only on unbaselined violations, prints suppressed ones attributed to their owning issue, prints each failing violation's fingerprint (so adding a baseline entry is copy-paste), and emits a::warningper stale entry.scripts/ally-review-consistency-baseline.json— new. The six violations measured live on 2026-09-01, each naming its PR, owning issue, and disposition. Fingerprints were generated mechanically from the real guard's output, not transcribed by hand..github/workflows/ally-review-consistency.yml— the header described a guard that fails on every violation, which is no longer true; it now describes the ratchet.scripts/check-ally-review-consistency.test.mjs— 86 → 114 tests.Why a baseline and not a staleness window
The issue offered scoping the audit to PRs that are non-draft,
mergeable_statein {CLEAN, UNSTABLE, BEHIND}, and updated within N days. Measured against this repo, that does not go green: #1525 ismergeable_state: behind— inside the allow-list — and was updated 2026-08-30, two days before filing, inside any plausible window. It stays in scope and the run stays red, failing the first acceptance criterion.The fingerprint gets the same property more precisely and for free. A PR that could actually merge on a bad attestation is one that is moving, and a moving PR changes its head, which breaks its own baseline entry. Staleness is a proxy for liveness; the head SHA measures it directly.
Verification
Unit suite (already wired into
pr.yml:282) — 114/114 pass:Covers: fingerprint stable under rewording; fingerprint changes under push, third-review escalation, different invariant, different PR; every
parseBaselinerejection path; a baselined PR cannot suppress a different violation on itself; and an assertion that the committed baseline suppresses exactly the measured set with zero stale entries.End-to-end against live repo state — both directions required by the issue.
(1) Green on current state —
node scripts/check-ally-review-consistency.mjs, exit 0:Note the findings are still printed and attributed, not hidden — the guard is not green because it stopped checking.
(2) Red on a violation the baseline does not cover — dropped the
#1304entry, re-ran against live data, exit 1:(3) Red on re-awakening — rewrote #1525's entry head
05325ee7to0a1b2c3dto simulate the PR being pushed to. The entry stops matching, both of that PR's violations are re-audited, exit 1, and the two now-stale entries only warn:node --test ./scripts/check-workflows-parse.test.mjsalso passes after the workflow header edit.Risks
::warning; pruning one never changes the verdict.violationFingerprintscrapes tokens from message text. If a future message shape defeats the scraping, the fingerprint changes and the run goes red — the safe direction. A rewording test guards the common case. Structuring the violations instead would have rewritten ~40 existing string assertions for no change in verdict.findPrViolationsis untouched; all six findings are real and still reported. Disposition of Release env lease + reconcile reservation on run cancel (BLO-21460) #1304 / fix: retry-now can now promote ccrotate_capacity parks (no retryOfRunId) #1316 / fix(alertmanager): route team=devops and recognize severity=page/ticket #1360 / fix(heartbeat): classify missing skills as deterministic failures (BLO-7991 AC3) #1525 belongs to their authors and is untouched here.Model Used
Claude Opus 4.5 (
claude-opus-4-5, Anthropic) via Claude Code / Claude Agent SDK — extended thinking, tool use (filesystem, bash, GitHub + Paperclip MCP). All verification commands above were executed, not simulated; the live guard output and the 114/114 test result are captured from real runs.Checklist
ally-review-consistencyandPEN-2847: none)Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template00b36ef4(2 skipped:security-review,Storybook visual regression, both N/A for a CI-only change)00b36ef4(5076163551) reports 0 critical / 0 important / 0 suggestions; recommended action "merge after the repository CI gates pass", which they now do. No Greptile review was requested on this repo surface.