Repository navigation
ci(precommit): gate merges on behavioural pre-commit hooks, not just formatter findings (#14878) - #14921
Merged
Conversation
…formatter findings (#14878)
This was referenced Aug 24, 2026
Contributor
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
4 tasks
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.
Thinking Path
The assignment was to make the Python suite gate. The first job was to establish why it does not, so I read the code before building anything — and most of it turned out to be already built.
The mechanism for #14353 has been merged since 2026-08-17 (PR #14402,
d0b2f42c3f):.github/workflows/python-required-context.ymlpublishes a job namedpython-suitewhen no Python path changed.github/filters/python-paths.ymlis the shared path set both workflows read, so the twoif:conditions cannot driftci.ymlcarries nopull_request.pathsfilter, so it always starts and can always reportrepo_tests/python_required_context_test.pyguards the arrangementSo the blocker is not missing code, not a name mismatch, not a swallowed ratchet, and not a per-shard context.
ci.ymlruns barepytest— a single failing test fails the shard, and the aggregator re-asserts it explicitly. The blocker is that the last step is a repository-settings change, and it has been sitting on the owner since 2026-08-17. That is a checklist at the bottom of this PR, not something a PR can deliver.I then verified the two objections that could make the flip unsafe, because if either held, flipping would deadlock every PR.
Objection 1 — "the aggregator check is intermittently absent." Recorded on #14353 from an audit of seven merges, one of which (#14455) showed all twelve shards green and no
python-suiteaggregator. It does not reproduce: on2834c6f038e6,python-suiteis present andsuccess. It was created at2026-08-17T22:30:20Z; the audit comment reporting it absent was posted at22:28:58Z. The aggregator had not been created yet. A timing artifact, not an absence — so nothing needs to key on the shard count instead.Objection 2 — "the shim's skip could mask a real failure." Two jobs publish the same context, so one of them decides it. Measured on live pull requests, by check-run
id(creation order), not by timestamp:ci.ymljobfailure97253091821skipped97250986170skipped97292839928success97293040244The substantive verdict is created last in both directions, and structurally so:
ci.yml's aggregator isneeds: [changes, python-shard], so its check run cannot exist until all twelve shards have finished, while the shim resolves in about a minute. Whichever rule GitHub applies to duplicate names — latest-wins, or all-must-pass withskippedcounted as success — it resolves to the real result. Thatskippedsatisfies a required context is directly evidenced on #14805, wherecode-qualityandstartup-import-smokeare both required and bothskipped.Swept the 60 most recently merged pull requests to check both claims at scale. Zero had no
python-suitecheck run. The shim's check run was created last in 6 of 60, and all 6 werereal=skipped, shim=success— the harmless direction. And the criterion this batch exists for is answered by a real merge rather than a contrived one: PR #14653 merged intoDev_new_guion 2026-08-22 withpython-suite=failure(shard 12/12 red, aggregator check run96991538736), five days after the shim landed.So
python-suiteis safe to require, and the only thing left is the settings flip. What this PR builds is the third issue in the batch, which is gateable today, plus the two loose ends the other two named.What Changed
#14878 — behavioural pre-commit hooks now block a merge.
enforce-precommit.ymlrunspre-commit run --all-filesand routes findings to::warning::. For formatters and linters that is right, and its comment says so.check_precommit_hooks_executed.py(#14181) already fails the job when a hook could not execute. Between them sits a third class neither covers:ssot-config-lib-guard, whose entry is a test suite — the regression guard for #14041. It executes fine, so #14181 is satisfied; it reports a finding, so the workflow warns and moves on. The guard ran in exactly one place and blocked nothing.pipeline-scripts/check_gating_precommit_hooks.py— fails the job when a hook on an explicit behavioural allowlist reports a finding. Keyed by hook id, not name, and self-guarding in both directions: an id that leaves.pre-commit-config.yamlfails rather than silently exempting itself, and an absent orSkippedresult fails rather than reading as clean.verify-precommit-configjob — which is already a required context. ci: the #14041 shell regression suite runs nowhere that gates — findings warn, and no local pre-commit hook is installed #14878 offered wrapping the suite under pytest as the alternative; that is the weaker option precisely becausepython-suiteis not required, so it would move the guard from one non-gating place to another.pipeline-scripts/check_gating_precommit_hooks_test.py— 12 tests, every one drivingmain()rather than a helper.CONTRIBUTING.md— records thatpre-commit installandscripts/install-git-hooks.share separate steps that both write.git/hooks/pre-commit, in what order to run them, how to verify both are live, and what the three hook classes mean for CI.#13162 loose end —
ci.yml's deployment-check step was namedSet up Python 3.12while pinning3.14. The name now carries no version, so there is no second copy to drift.The allowlist has one entry today because exactly one hook meets the criterion — its entry is a test suite asserting shipped behaviour, so its verdict is independent of the diff. The criterion is written down in the script header rather than implied, and the list is guarded, so the next one is an append.
Verification
Mutation, on the real CI transcript. Pulled the actual
precommit-output.txtfrom theverify-precommit-configjob of run32679083207(job97292541874, basee8cb8e7ae), stripped the log timestamps to reproduce the file the step reads, and ran the gate against it in a scratch copy outside the repository tree. Nothing was committed or pushed.Shell SSOT Library Self-Test (#14041)→Passed)FailedSkipped0 → 1on the single mutated word, with the control still green. The four fail-closed directions all block, so an absent, skipped or unparseable result cannot read as a clean one.Baseline confirmed before gating. On base
e8cb8e7aethe guard reportsPassed, so this change does not redden any open pull request on day one. That was checked from the CI log, not assumed.Unit tests: 12 passed, in a layout-faithful scratch copy under Python 3.10.12 — a local interpreter figure, not a claim about CI's 3.14. They also run here in
python-suite.Not verified locally: nothing in this repository was executed in place, nothing was installed, and no branch protection or repository setting was touched.
A note found while building this, recorded because it makes hook names unusable as identifiers: several hook names are written unquoted with a space before a
#issue reference, so YAML reads the rest of the line as a comment.name: Function Length Check (Issue #5512)parses toFunction Length Check (Issue, and that truncated string is what pre-commit prints. The allowlist resolvesid -> namethrough the same parser, so it matches what actually lands in the log. Filed as #14923 rather than fixed here.Model Used
Claude Opus 5 (1M context)
Owner action required — repository settings, not code
python-suitecannot be made to gate from a pull request. Adding a required context is a branch-protection change onDev_new_gui. Everything up to that line is merged; this is the line.python-suite(exact name, lower-case, hyphenated) to branch protection onDev_new_gui, taking the list from 10 contexts to 11.One asymmetry found while verifying this, filed as #14922: the
Unit & Integration Testspair is ordered the opposite way round — in 34 of the 60 merged pull requests swept, the shim's check run was created after the real frontend job's. No merge went out with the real frontend suite red, so nothing is observably broken, but that pair is not protected the way this one is.Evidence supporting the flip is in the Thinking Path above: the shim exists and is fail-closed, the base branch is green (twelve consecutive
successruns ofci.ymlonDev_new_gui), and the duplicate-name ordering resolves to the real verdict in both directions.Closes #14878
Refs #14353, #13162