compliance-audit checks that rulesets EXIST, never that their contents match the codified standard

[don-petry]

The ask, and the correction

The request was to "implement and enforce required_review_thread_resolution org-wide, documented and rolled out via the compliance mechanism."

It is already implemented and already rolled out. Verified today:

• Codified: standards/rulesets/pr-quality.json:28 → "required_review_thread_resolution": true
• Applied: 11/11 repos in the org enforce it right now (.github, .github-private, bmad-bgreat-suite, broodminder-export, broodly, google-app-scripts, markets, incubator, TalkTerm, ContentTwin, repo-template)
• Rolled out by: scripts/apply-rulesets.sh, which reads the codified JSON and converges each repo (per-repo replication is deliberate — org-level rulesets need a Team plan and this org is Free; Consolidate per-repo replicated rulesets → native org-level rulesets (when on GitHub Team) #611 tracks that).

Two consequences worth stating plainly:

1. There is nothing to implement for thread resolution itself.
2. It would not have prevented the regression that prompted this. Thread resolution was already on for .github when feat: implement issue #756 — Provision <id>:hands-off labels so persona opt-out actually works #757 merged with a known defect. The bug there (PR comments from maintainers are mechanically inert — they neither block merge nor reach dev-lead .github-private#1290) is that issue comments create no thread at all, so thread resolution never applies to them. Turning on a setting that is already on fixes nothing.

The real gap this exposed

check_rulesets verifies a ruleset EXISTS BY NAME. It never compares its contents to the codified JSON.

names=$(echo "$rulesets_json" | jq -r '.[].name')
if ! echo "$names" | grep -qx "pr-quality"; then
  add_finding "$repo" "rulesets" "missing-pr-quality" "error" ...

That is the whole check, plus check_ruleset_bypass_actors and check_legacy_rulesets. So today:

Anyone can flip required_review_thread_resolution to false on any repo — or drop require_code_owner_review, or lower required_approving_review_count to 0 — and compliance still passes, because a ruleset named pr-quality still exists.

The codified standard and live reality can diverge arbitrarily and silently. apply-rulesets.sh converges a repo when you run it; nothing detects drift between runs. This is the known "pr-quality drift" follow-up from the #575 relocation, never filed until now.

It is also, again, the shape #755 keeps surfacing: a green check that proves less than it appears to. "Rulesets: ✅" reads as "the ruleset is correct". It means "a ruleset with that name exists."

Scope — every parameter, not just this one

Drift is currently invisible for all of pr-quality.json and code-quality.json:

• required_review_thread_resolution
• required_approving_review_count
• require_code_owner_review
• dismiss_stale_reviews_on_push
• require_last_push_approval
• allowed_merge_methods
• required_status_checks (code-quality)

Task

Extend check_rulesets in scripts/compliance-audit.sh to compare each live ruleset against its codified source in standards/rulesets/*.json and raise a finding per drifted parameter.

Constraints

• The codified JSON is the source of truth (Relocate org-wide compliance rulesets (code-quality, pr-quality) from .github-private to .github/standards/rulesets/ #575/Retire/delineate the detection-based apply-rulesets.sh — it diverges from the codified ruleset (#575 debt) #580). Never infer intent from the live ruleset — that is exactly the detection-based builder that apply-rulesets.sh retired for diverging from the standard. Read standards/rulesets/pr-quality.json and code-quality.json.
• Normalisation is the hard part, and where this will go wrong. The API returns fields the codified JSON does not carry (id, source, source_type, created_at, _links, actor ids), key order differs, and some parameters are returned only when set. Compare the parameters we codify — do not diff whole blobs and report noise. Getting this wrong means either false findings on every repo (and the audit gets ignored) or a comparison so loose it detects nothing.
• Fail closed. If a ruleset cannot be fetched or parsed, that is a finding — not a pass. An error must never read as "no drift". That conflation is the systemic bug in Persona framework — pre-rollout review findings #755 and has already shipped twice (feat: implement issue #756 — Provision <id>:hands-off labels so persona opt-out actually works #757, and the manifest fetch in Add the persona @-mention router #754).
• Findings must name the parameter, the expected value, and the actual value. "pr-quality drifted" is not actionable at fleet scale.
• Reuse the existing add_finding shape and the standards/github-settings.md#... doc anchors.
• Do not auto-remediate. This is the audit; apply-rulesets.sh is the applier, and keeping them separate is the design.

Files that exist today (verified)

• scripts/compliance-audit.sh — check_rulesets() at ~line 498; check_ruleset_bypass_actors, check_legacy_rulesets alongside it
• standards/rulesets/pr-quality.json, standards/rulesets/code-quality.json — the codified source of truth
• scripts/apply-rulesets.sh — the applier that converges to those files
• test/scripts/compliance-audit/ — the bats suite (run by .github/workflows/compliance-audit-tests.yml)
• standards/github-settings.md — the doc anchors findings point at

Acceptance criteria

• A repo whose pr-quality ruleset has required_review_thread_resolution: false produces a compliance finding naming the parameter, expected, and actual.
• Same for a lowered required_approving_review_count and a dropped require_code_owner_review.
• Drift in code-quality's required_status_checks is detected.
• All 11 repos pass today — they are in sync, so a clean fleet must stay clean. False findings here make the audit worthless.
• A fetch/parse failure is a finding, not a pass.
• bats coverage in test/scripts/compliance-audit/ for: in-sync (no finding), each drifted parameter (finding), and fetch failure (finding).
• shellcheck --severity=warning -x scripts/compliance-audit.sh clean.
• standards/github-settings.md states that ruleset contents are audited, not just presence.

Ground truth

$ gh api repos/petry-projects/.github/rules/branches/main --jq '[.[]|select(.type=="pull_request")|.parameters]'
[{"required_approving_review_count":1,"require_code_owner_review":true,
  "dismiss_stale_reviews_on_push":true,"require_last_push_approval":true,
  "required_review_thread_resolution":true, ...}]

[don-petry]

Dev-Lead Implementation Plan

Issue: #766 — compliance-audit checks that rulesets EXIST, never that their contents match the codified standard

Scope

Add a check_ruleset_contents step to scripts/compliance-audit.sh that compares each live pr-quality / code-quality ruleset against its codified source in standards/rulesets/*.json and raises one finding per drifted parameter (naming parameter, expected, actual), failing closed on any fetch/parse error.

Comparison semantics (grounded in standards/github-settings.md)

• pull_request scalars (required_approving_review_count, require_code_owner_review, dismiss_stale_reviews_on_push, require_last_push_approval, required_review_thread_resolution) → exact match vs codified.
• allowed_merge_methods → exact set match (doc: ruleset enforces squash-only; a widened set is drift).
• required_status_checks → subset check: every codified context must be present; repo-specific additions (build-and-test, Go, coverage, …) are explicitly allowed by the standard and are NOT drift. A missing codified context IS drift.
• Only codified keys are compared (API-only fields like id, source, required_reviewers, dismissal_restriction are ignored) → no normalisation noise.
• Fail closed: unfetchable/invalid ruleset JSON, missing codified source file, or a ruleset that exists by name but lacks the codified rule type → a finding, never a silent pass.

Note on the "all 11 in sync" premise

Verified against the live fleet today: required_review_thread_resolution is indeed true on all 11 repos (matches the issue). However several repos already carry genuine drift on other codified parameters (e.g. broodly/ContentTwin/markets have require_code_owner_review:false; a few widened allowed_merge_methods; markets/bmad are missing a mandatory required check). This check surfaces that pre-existing, previously-invisible drift — which is the entire point of the issue. Unit tests pin behaviour with controlled fixtures.

Implementation Checklist

• Add RULESETS_SRC_DIR global + check_ruleset_contents / _ruleset_contents_one to scripts/compliance-audit.sh, wired into check_rulesets
• Reuse existing add_finding shape + standards/github-settings.md#... anchors
• Update standards/github-settings.md to state ruleset contents are audited, not just presence

Tests to Write (test/scripts/compliance-audit/ruleset-contents-drift.bats)

• in-sync pr-quality+code-quality → no drift finding
• required_review_thread_resolution:false → finding (param/expected/actual)
• lowered required_approving_review_count → finding
• dropped require_code_owner_review → finding
• code-quality missing a codified required check → finding (subset)
• code-quality with EXTRA checks → NO finding (subset allows additions)
• widened allowed_merge_methods → finding
• ruleset detail fetch failure → finding (fail closed)

Test Command

bats test/scripts/compliance-audit/ and shellcheck --severity=warning -x scripts/compliance-audit.sh

[don-petry]

Dev-Lead: Implementation Complete

Plan Execution

• Added check_ruleset_contents / _ruleset_contents_one + RULESETS_SRC_DIR to scripts/compliance-audit.sh, wired into check_rulesets
• Compares each live pr-quality / code-quality ruleset against its codified source in standards/rulesets/*.json; one finding per drifted parameter (parameter, expected, actual)
• Fail-closed on unfetchable/invalid ruleset, missing/invalid codified source, or a ruleset that lost its codified rule type
• Updated standards/github-settings.md to state ruleset contents are audited, not just presence
• Added test/scripts/compliance-audit/ruleset-contents-drift.bats (10 tests)

Comparison semantics (grounded in standards/github-settings.md)

• pull_request scalars + allowed_merge_methods → exact match vs codified (arrays order-insensitive). The doc states the ruleset enforces squash-only, so a widened merge-method set is drift.
• required_status_checks → subset: every codified context must be present, but repo-specific additions (build-and-test, Go, coverage, …) are explicitly allowed by the standard and are not drift; a missing codified context is drift.
• Only codified keys are compared, so API-only fields (id, source, required_reviewers, dismissal_restriction, actor ids, key order) never produce noise.

Test Results

1..10
ok 1 in-sync pr-quality + code-quality → no drift finding
ok 2 required_review_thread_resolution:false → finding naming the parameter
ok 3 lowered required_approving_review_count → finding
ok 4 dropped require_code_owner_review → finding
ok 5 widened allowed_merge_methods → finding
ok 6 code-quality missing a codified required check → finding
ok 7 code-quality with EXTRA repo-specific checks → NO finding (subset allows additions)
ok 8 code-quality detail fetch failure → finding (fail closed)
ok 9 pr-quality detail fetch failure → finding (fail closed)
ok 10 pr-quality present but missing its pull_request rule → finding

Full suite: bats test/scripts/compliance-audit/ → 151 passing, 0 failing. shellcheck --severity=warning -x scripts/compliance-audit.sh clean. .dev-lead/scripts/dev-lead-lint.sh passes.

Files Changed

• scripts/compliance-audit.sh: new check_ruleset_contents/_ruleset_contents_one + RULESET_DRIFT_JQ diff program + RULESETS_SRC_DIR, called from check_rulesets.
• standards/github-settings.md: new "Compliance check — contents, not just presence (compliance-audit checks that rulesets EXIST, never that their contents match the codified standard #766)" note.
• test/scripts/compliance-audit/ruleset-contents-drift.bats: new suite (in-sync, each drifted param, subset additions allowed, missing-rule, fetch failure).

Notes — the "all 11 in sync" premise was only true for required_review_thread_resolution

I validated the new check against the live fleet. required_review_thread_resolution is true on all 11 repos (matches the issue). But other codified parameters have genuine, pre-existing, previously-invisible drift — which is exactly what #766 says the old presence-only check let through. The check reports it correctly (no false positives; repos that merely add checks stay clean):

Repo	Content findings
.github-private, broodminder-export, incubator, google-app-scripts	none (clean)
.github, TalkTerm, repo-template	allowed_merge_methods widened beyond squash-only
.github, bmad-bgreat-suite	code-quality missing dependency-audit / Detect ecosystems
broodly, ContentTwin	require_code_owner_review, dismiss_stale_reviews_on_push, require_last_push_approval all false
markets	the three pr-quality booleans above and code-quality missing agent-shield / AgentShield + dependency-audit / Detect ecosystems

These are true findings, not false positives — remediate by running scripts/apply-rulesets.sh --repo petry-projects/<repo> (kept separate from the audit by design; this PR does not auto-remediate). Recommend opening follow-up remediation issues for the drifted repos.