Make configure.sh check mode verify what the payloads declare - #540
Merged
Conversation
Three findings from #538, each surfaced by a reviewer reading the canonical as it landed in five repos, and each re-verified against this copy before being fixed. check_settings streamed the payload from a process substitution. A jq failure there leaves the loop body unexecuted without tripping set -e, so every static setting reported as checked and passing while nothing was compared. The payload is now parsed into a variable first, a parse failure is a loud FAIL, and a payload that yields no keys is a FAIL rather than a clean run, since a gate that finds nothing is indistinguishable from a gate with nothing to find. check_ruleset compared rule types in one direction, so a rule added live that no payload declared passed as clean. The rule-type set is now compared both ways. check_ruleset compared parameters for two rule types and named their fields one at a time, so review-thread resolution, stale-review dismissal and the status-check policy flags went unverified though the payloads declare them. Every parameterized rule is now compared on its whole parameters object, which keeps the check payload-driven: a parameter added to a payload is audited with no change here. The header claimed every applied ruleset and setting must match while the code compared a fraction. It now states what is verified and, as importantly, what is not. Verified by negative test rather than by a clean pass, since a clean pass on a conformant repo proves nothing. Removing a rule from a payload copy fails the rule-set assertion, flipping copilot_code_review.review_on_push fails the parameters assertion, and a malformed settings.json fails with exit 1 where it previously reported a clean audit. A live check against this repo passes and now additionally verifies copilot_code_review parameters on both rulesets. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens repo-config/configure.sh’s check mode so it verifies what the committed payloads actually declare (ruleset rule-type set in both directions, and parameters for all parameterized rules), closing the gaps where check could previously report a false clean run.
Changes:
- Update
check_rulesetto assert rule-type set equality (payload ↔ live) and to compare the fullparametersobject for every parameterized rule (with sorted JSON keys). - Make
check_settingsfail loudly on malformed/emptysettings.jsonby parsingjqoutput before looping (avoids the “process substitution hides jq failure” footgun). - Clarify header documentation to precisely describe what
checkdoes and does not verify.
The whole-object compare dropped an order-insensitivity the previous code had explicitly: it sorted allowed_merge_methods and compared required_status_checks by context. Comparing the objects directly reintroduced array order as a source of false drift, since the API guarantees no order for either. Both sides are now normalized before comparison, sorting a scalar array directly and sorting required_status_checks by context, its identifying field. The normalization is a jq walk rather than a per-field rule, so it keeps the payload-driven property the whole-object compare was adopted for. Verified both directions: reversing allowed_merge_methods and the required_status_checks list in a payload copy passes, and changing a merge method to an actual different value still fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 3, 2026
ptr727
added a commit
to ptr727/homeassistant-purpleair
that referenced
this pull request
Aug 3, 2026
`.markdownlint-cli2.jsonc` is carried `verbatim` from ptr727/ProjectTemplate, so a copy that differs is drift rather than a choice. The hub's regenerated fleet divergence report lists this repo for it. The canonical enables `MD033` with `details` and `summary` allowed, which is a behavior change rather than a comment sweep. Verified rather than assumed, since that is the kind of change that fails CI after the fact: `markdownlint-cli2` over `**/*.md` reports **0 issues** here under the restored config. Line endings preserved. ## `repo-config/configure.sh` was withdrawn from this pull request An earlier revision of this branch also re-vendored `repo-config/configure.sh`, and this description still described it after the file was removed. That was a stale description rather than a missing commit, and it is corrected here. The reason it was withdrawn is worth stating, because it is the blocker for this repo: the canonical script resolves its ruleset payloads as `develop.json` and `main.json`, and this repo carries `ruleset-develop.json` and `ruleset-main.json`. Copying the script in leaves `apply` and `check` aborting on payloads that do not exist, which is worse than the older script it replaced. The hub's divergence ledger recorded the filename fork, so the information was available and the re-vendor ran without acting on it. Converging this repo needs the payload migration with its content reconciled against the canonical, which is judgment rather than a file copy, so it gets its own pull request. Known defects in the canonical script are tracked at ptr727/ProjectTemplate#538 and fixed in ptr727/ProjectTemplate#540, which should land before that migration so this repo takes a corrected script rather than the current one twice. Part of the fleet re-vendor sweep tracked in the hub's `TODO.md`. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 3, 2026
ptr727
added a commit
that referenced
this pull request
Aug 3, 2026
#543) Fixes the finding on the promotion #542. A promotion's head is `develop`, which is protected, so the fix lands here and #542 picks it up when this merges. ## The finding > The header comment says check mode verifies "exactly what the payloads declare" and that anything absent from settings.json is unaudited, but the script also asserts derived/dynamic settings (has_discussions, default_branch) and security toggles in addition to payload-driven rulesets/static settings. Correct on the facts. `check_settings` asserts `has_discussions` (computed from `private`) and `default_branch` (when `main` exists), and `check_security` asserts the two Dependabot features. None of those comes from a payload. ## Why it happened, which is the part worth recording This is an **over-correction of the defect #540 fixed**, not a new one. The header used to over-promise: "every applied ruleset, setting, and security feature must match", while the code compared a fraction. #540 rewrote it to "exactly what the payloads declare", which fixed the over-claim by introducing an under-claim. A coverage note that is wrong in *either* direction misleads the person reading it to decide what still needs checking by hand. Over-claiming tells them to skip something unverified. Under-claiming tells them to re-check something already covered, and erodes trust in the rest of the note. Landing on the true statement took two passes and a reviewer, which is the honest record. ## The change The header now names the three groups `check` actually asserts, says which one is payload-driven, and states the single genuine gap rather than implying a larger one: - rulesets and static settings, driven by the committed payloads - the derived settings `apply` computes, asserted by name - the two Dependabot security features, asserted the same way - unaudited: a static setting absent from `settings.json`, because only that group is payload-driven ## Verification `bash -n` clean, `prose_lint` clean, and a live `check` against this repo still passes with all three groups asserted. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
ptr727
added a commit
that referenced
this pull request
Aug 3, 2026
…ndor groundwork (#542) Six commits, all from one session. Merged as a **merge commit**, never squashed, per the branching model. | Commit | PR | What it does | | --- | --- | --- | | `1ab9b32` | #537 | Deletes three stale PhotoCleaner `driftNotes` that described finished work | | `bf746aa` | #535 | Buckets `TODO.md` by the surface each entry changes, records five issues verified complete, refreshes the divergence ledger | | `69276d1` | #536 | Adds the byte-locked `AGENTS.md` "Fleet Bootstrap" section, the host-wide `fleet-bootstrap` block, and the `AUDIT.md` routing that sends an un-stood-up repo to `STANDUP.md` first | | `530dc0d` | #539 | Records two `gh-write-guard` limits found by running it: a write inside a script is unseen, and a push followed by a newline over-blocks | | `5f4c17f` | #540 | Makes `configure.sh check` verify what the payloads declare, closing a false clean where a `jq` failure left every setting reported as passing while nothing was compared | | `a549572` | #541 | Reports a verbatim section that never arrived as **absent** rather than as hand-modified | ## Why this promotion matters more than most Two of these change what the fleet is measured against, so until they reach `main` every downstream audit compares against ground truth that predates them. **#536 is the one with reach.** A downstream agent had no way to discover this repository. `AGENTS.md` is carried at `intent`, and the hub's only self-reference described *the hub*, which is false about a downstream repo, so an agent adapting the file correctly deleted it. Four repos now hold no hub reference at all and one holds no `AGENTS.md` either. The replacement is `verbatim` so adaptation cannot remove it, declared in `spec/files.json` and classified in `spec/section-model.md` so the audit reports its absence as drift rather than losing it silently. **#540 closes a false clean.** `check_settings` streamed its payload from a process substitution, and a `jq` failure there leaves the loop body unexecuted without tripping `set -e`. Every static setting reported as checked and passing while nothing was compared. Verified by negative test rather than by a clean pass: a malformed `settings.json` now fails with exit 1, an extra live rule fails the rule-set assertion, and a changed `copilot_code_review` parameter fails the parameters assertion. **#541 corrects a report that was actively misleading.** It accused seventeen repos of hand-editing `AGENTS.md` sections they had never been given. They still hold the pre-split monolithic file, so the sections are simply absent. A hand-edit is reconciled against a local decision, an absence is carried, and the report was naming the wrong one. ## Fleet state behind this Seventeen downstream re-vendor pull requests merged alongside this work, each gate-checked at merge time and each verified after. Four repos had `configure.sh` withdrawn from their re-vendor when review found they carry `ruleset-*.json` payload names the canonical does not resolve, which would have left `apply` and `check` aborting on files that do not exist. That payload migration is still owed and is tracked in `TODO.md`. ## Expected immediately after this merges The divergence ledger currently reports the hub itself as not carrying "Fleet Bootstrap" and as owing a `configure.sh` re-vendor. That is the ledger reading each repo's `main`, honestly, before this promotion. Regenerating after the merge is what makes the report meaningful again, and the `configure.sh` re-vendor list will collapse as repos take the corrected script. ## Verification `spec/validate.py` OK (21 cataloged), diff-scoped `prose_lint` clean, `markdownlint-cli2` clean, editorconfig clean, `gh-write-guard --selftest` PASS, 157 `prose_lint` unit tests pass.
ptr727
added a commit
that referenced
this pull request
Aug 3, 2026
Generated output only, no hand edits. `reports/divergences.md` reads each repo's ground-truth `main`, so before #542 promoted it was measuring a hub `main` that predated this session and reporting the hub as not carrying a section it had just authored. ## What moved, and how each was established **Predicted and correct.** `AGENTS.md > Fleet Bootstrap` no longer lists ProjectTemplate. The hub's own `main` carries it now, so that row was pre-promotion state rather than fleet drift. **Predicted and wrong.** I expected `repo-config/configure.sh` to fall from 9 repos to 7. It fell to **8**. ProjectTemplate dropped off correctly, but PhotoCleaner is **genuinely stale** rather than an artifact: #540 moved the canonical, so a copy that was current yesterday is behind today. That is the intended consequence of changing a `verbatim` file, flagged in #540 itself and then forgotten when predicting here. **Not predicted, and checked rather than assumed.** PhotoCleaner dropped off five carried-section stale rows and off the `GOVERNANCE.md > Representative Data` not-carried row. The cause is external to this work: that repo merged its own promotion `c457ff3` earlier today and its `main` now carries the section. An unexplained improvement deserves the same scrutiny as an unexplained regression, so it was verified against that repo's commits rather than accepted. ## What the report now says about the fleet - **19 repos owe `AGENTS.md > Fleet Bootstrap`.** The bootstrap shipped and nothing downstream has it. That is the propagation job #536 exists to make possible, and it is the honest measure of where the fleet stands. - **Financial-Modeling is the only repo still stale on the carried governance sections**, at 18 rows. - **`.markdownlint-cli2.jsonc` still lists 16 repos**, because this session's fleet merges landed on each repo's `develop` and have not promoted to their own `main`. The ledger reads `main`, correctly, so these clear as those repos promote. - **`repo-config/configure.sh` at 8**, five of which additionally need the `ruleset-*.json` payload migration before they can take the canonical at all. ## Verification Regenerated by `python3 spec/fidelity_honesty.py --report` from a live fleet pass after the promotion merged. editorconfig clean, CRLF preserved, diff-scoped `prose_lint` clean. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
ptr727
added a commit
that referenced
this pull request
Aug 3, 2026
The report cited #536, #540 and #543 through `issue-*` link labels while their definitions pointed at `/pull/` URLs, so the label contradicted what it resolved to. #456 is a real issue and keeps its label. The External group is alphabetized, which the ordering had drifted from once `pr-545` was appended. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 3, 2026
Closed
ptr727
added a commit
that referenced
this pull request
Aug 3, 2026
Closes #549, raised by the Blog agent from a Copilot review on a downstream re-vendor and declined there because `configure.sh` is carried `verbatim`. ## The defect The payload-driven ruleset comparison added in #540 built its normalizer on `walk/1`, which arrived in **jq 1.6**. On a host carrying jq 1.5 the filter does not degrade — it **fails to compile**, so `check_ruleset` reports drift on every parameterized rule it never actually compared. That is the inverse of the false clean #540 was written to close, and arguably worse: a false *failure* teaches an operator to distrust the tool, where a false pass merely fails to warn them. ## Verified on a real jq 1.5, not argued ```console jq version: jq-1.5-1-a5b5cbe --- OLD filter (calls walk): def n: walk(if type=="array" then sort else . end); n jq: 1 compile error --- NEW filter (defines its own): {"allowed_merge_methods":["merge","squash"],"nested":{"deep":[1,2,3]},"required_status_checks":[{"context":"a"},{"context":"b"}]} ``` The reported failure reproduces exactly, and the fix compiles and returns the sorted document. Output is byte-identical to jq 1.7 on the same input, including a deliberately nested array to confirm the local definition still recurses. Finding a genuine jq 1.5 took three attempts — `imega/jq:1.5` does not exist, Debian buster's archives are gone, and Alpine 3.8 ships a master build that already has `walk`. Ubuntu 18.04 has it. Worth recording, because "I could not reproduce it" would have been the wrong conclusion from the first two. ## The choice `walk` is defined inside the filter, so the script calls nothing jq 1.5 lacks and keeps one code path across versions. The alternative in the issue — assert a jq version up front and fail with a message — was considered and declined for the reason the issue itself gives: `check` is the read-only mode, and refusing to run at all is a worse outcome than running. The issue stated that as a preference rather than a decision, and I agree with it. ## Verification `bash -n` clean, diff-scoped `prose_lint` clean, and a live `check` against this repo still passes on all three parameterized rules of both rulesets. ## Downstream `configure.sh` is carried `verbatim` with `appliesTo: "*"`, so this joins the re-vendor the ledger already lists. It is the same file that generated a re-vendor three times today, which is precisely the evidence behind the vendored-tooling entry added in #546. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the three code findings in #538. Each was raised by a reviewer reading the canonical as it landed in five downstream repos, and each was re-verified against this copy before being touched.
The unifying defect:
checkis what a maintainer runs to confirm a repo is configured correctly, and it compared a fraction of whatapplysets.1.
check_settingscould report a clean audit having compared nothingA
jqfailure inside process substitution leaves the loop body unexecuted, andset -edoes not trip on it. Every static setting then reported as checked and passing while nothing was compared. The payload is now parsed into a variable first, a parse failure is a loudFAIL, and a payload yielding no keys is aFAILrather than a clean run, since a gate that finds nothing is indistinguishable from a gate with nothing to find.2. An extra live rule was invisible
The rule-type comparison ran one way: payload types had to exist live, and nothing checked the reverse. A rule someone added by hand in the UI,
required_linear_historyonmainsay, passed as clean. The set is now compared both directions.3. Most rule parameters were never read
Parameters were compared for two rule types, field by field. So
copilot_code_review.parameters,required_review_thread_resolution,dismiss_stale_reviews_on_push,strict_required_status_checks_policyanddo_not_enforce_on_createwere all declared in the payloads and never verified.Every parameterized rule is now compared on its whole parameters object, with keys sorted on both sides so API key order cannot read as drift. That keeps the check payload-driven: a parameter added to a payload is audited with no change here, which is the property naming fields one at a time had quietly lost.
The header, which was the reason none of this was obvious
It claimed "every applied ruleset, setting, and security feature must match" while the next line partially walked it back. It now states what is verified and, as importantly, what is not: anything the payloads do not declare is unaudited by construction.
Verification, by negative test
A clean pass on a conformant repo proves nothing, so each assertion was made to fail on purpose:
required_signaturesfrom amain.jsoncopyFAIL 'main' rule set = ...copilot_code_review.parameters.review_on_pushFAIL 'main' rule 'copilot_code_review' parameters match the payloadsettings.jsonFAIL ... did not parse, exit 1 (previously a clean pass)settings.jsonFAIL ... declares no keysA live
checkagainst this repo passes and now additionally verifiescopilot_code_reviewparameters on both rulesets, which nothing verified before.bash -nclean,prose_lintclean, editorconfig clean, LF preserved per.gitattributes.Not in this PR
#538's fifth finding, that the script names
spec/secrets.jsonin three places while being carriedverbatiminto repos that have nospec/, is left alone deliberately. It is the same open question as theTODO.mdentry "Decide where a carried file may name hub-only machinery", which currently cites thescripts/pr_review.pycase, and both should be settled together rather than one being patched here.Downstream consequence
This file is carried
verbatimwithappliesTo: "*", so every repo holding a copy is now stale against the hub and takes this on its next re-vendor. That is the intended order: five repos additionally need aruleset-*.jsonpayload migration before they can take the script at all, and doing that before this landed would have re-vendored them twice.