configure.sh check mode reports a clean audit over drift it never compares

[ptr727]

Raised from the fleet re-vendor sweep, where Copilot reviewed the canonical repo-config/configure.sh as it landed in five repos and found the same class of defect from five different angles. Every claim below was re-verified against the hub's own copy on develop at 1ed0cc8 rather than taken from the downstream reviews.

The unifying problem: check is the read-only inverse of apply and is what a maintainer runs to confirm a repo is configured correctly, but it compares a fraction of what apply sets. A live ruleset can drift in ways that matter and check still prints a clean pass.

1. Rule parameters are mostly uncompared

check_ruleset asserts that every rule type in the payload exists live, then compares parameters for exactly two of them: pull_request.allowed_merge_methods and the required_status_checks context list.

Everything else in the payloads is unchecked, including:

• copilot_code_review.parameters (for example review_on_push)
• pull_request beyond merge methods (required_review_thread_resolution, dismiss_stale_reviews_on_push)
• required_status_checks beyond contexts (strict_required_status_checks_policy, do_not_enforce_on_create)

All of these are present in the committed payloads, so the payload already states the intent that check declines to verify.

2. An extra live rule is invisible

The presence loop is one-directional: payload types must exist live, and nothing asserts the reverse. A rule added to a live ruleset that the payload never declared (say required_linear_history on main) passes a clean audit. Drift that a human introduced by hand in the GitHub UI is exactly the drift this script exists to catch.

3. The header over-promises what check verifies

Two adjacent lines disagree:

The check mode is the read-only inverse, where every applied ruleset, setting, and security feature must match.

That also survives the GitHub API normalizing a stored ruleset, comparing rule presence, merge methods and required checks rather than a byte diff.

The second sentence is accurate and the first is not. An operator reads the first.

4. check_settings can silently become a no-op

done < <(jq -r 'to_entries[] | "\(.key)\t\(.value)"' "$settings_file")

If jq fails, for instance on a malformed settings.json, the loop body never runs. set -e does not reliably trip on a failure inside process substitution, so every static setting reports as checked and passing while nothing was compared. This is the worst finding here, because it is a false clean rather than a partial one, and it is the same shape as the prose_lint.py scope floor already tracked in TODO.md: a gate that finds nothing is indistinguishable from a gate with nothing to find. Capture the jq output first and fail loudly if it does not parse.

5. It names hub-only machinery a downstream repo cannot open

Three places point the reader at spec/secrets.json:

• the header comment
• the cmd_check comment
• the note printed to the operator

configure.sh is carried verbatim with appliesTo: "*", and spec/ is hub-only, so in every downstream repo that note points at a path that does not exist. This is a second instance of the TODO.md entry "Decide where a carried file may name hub-only machinery", which currently cites only the scripts/pr_review.py case, so the two should be settled together.

Not a defect, recorded so it is not re-raised

Reviews on two repos flagged that REPO=owner/repo ./repo-config/configure.sh apply no longer works while repo-config/README.md documents it. The hub script does not honor REPO and the hub README does not document it, so this is stale prose in those repos' older carried READMEs rather than a hub defect. It resolves when they take the current canonical.

Sequencing

The fleet re-vendor of this file is in flight and paused on this. Fixing the script here first is cheaper than re-vendoring twice, and four repos additionally need a payload-filename migration (ruleset-*.json to the canonical names) before they can take the canonical at all.

[ptr727]

Re-measured against develop @ 2951273. Four of the five findings are fixed; trimming this to the one that stands.

#	Finding	State
1	check_ruleset compared parameters for only two rule types	fixed — configure.sh:22: "the whole parameters object of every parameterized rule"
2	presence loop one-directional, so a hand-added live rule passed clean	fixed — configure.sh:217: "The live rule-type set must equal the payload's, compared in both directions."
3	header contradicted the adjacent accurate sentence	fixed — configure.sh:20 now reads "it verifies the same three groups apply writes"
4	done < <(jq …) no-ops into a false clean when jq fails	fixed — configure.sh:272 is now if ! pairs="$(jq -r … )"; then
5	a verbatim / appliesTo: "*" file names hub-only spec/secrets.json	live

Finding 5 as it stands today — three sites, all in the carried script:

• repo-config/configure.sh:29 — "Secrets are per-repo (see spec/secrets.json) and not checkable from a standalone carry"
• repo-config/configure.sh:310 — "Secrets are per-repo (spec/secrets.json) and not readable by value."
• repo-config/configure.sh:312 — the note string a downstream operator actually reads: "verify manually: the repo's required secrets (see spec/secrets.json) are present with valid values"

Line 312 is the one that bites, since it is emitted output rather than a comment, pointing a downstream reader at a path their repo does not carry.

This remains an instance of the open TODO.md question of where a carried file may name hub-only machinery, which so far cites only scripts/pr_review.py. Two instances is enough to want the rule stated rather than decided twice.

Retitling suggestion, if you want the issue to match its residue: "A carried verbatim file points downstream readers at hub-only spec/secrets.json".

[ptr727]

Complete on all three points, verified point by point against develop at 3d1a0b1.

1. Rule parameters are mostly uncompared. check_ruleset now compares the whole parameters object of every parameterized rule rather than two named fields, so copilot_code_review.parameters, required_review_thread_resolution, dismiss_stale_reviews_on_push, strict_required_status_checks_policy and do_not_enforce_on_create are all audited:

want="$(jq -S -c --arg t "$t" "[.rules[] | select(.type==\$t) | .parameters] | first | $norm" "$file")"
got="$(jq -S -c --arg t "$t" "[.rules[] | select(.type==\$t) | .parameters] | first | $norm" <<<"$live")"
assert "'$rname' rule '$t' parameters match the payload" test "$got" = "$want"

Keys and set-like arrays are sorted on both sides, so API key order and array order cannot read as drift, and the comparison stays payload-driven: a parameter added to a payload is audited with no code change.

2. An extra live rule is invisible. The rule-type set is now compared in both directions:

# The live rule-type set must equal the payload's, compared in both directions.
# Checking only that each payload type is present live misses a rule someone added by hand.

so a required_linear_history added by hand in the UI fails the check rather than passing it.

3. The header over-promises. The false sentence is gone. The header now reads that "The check mode is the read-only inverse, and it verifies the same three groups apply writes", describes the ruleset comparison accurately, and states the limit outright: "What is unaudited is a static setting absent from settings.json, since only that group is payload-driven", with secrets called out as a manual-verify note.

Landed by #540 and #543.