Skip to content

ci(precommit): gate merges on behavioural pre-commit hooks, not just formatter findings (#14878) - #14921

Merged
mrveiss merged 3 commits into
Dev_new_guifrom
issue-14353-python-suite-gates
Aug 24, 2026
Merged

mrveiss merged 3 commits into
Dev_new_guifrom
issue-14353-python-suite-gates

Conversation

@mrveiss

@mrveiss mrveiss commented Aug 24, 2026 •

Copy link
Copy Markdown
Owner

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.yml publishes a job named python-suite when no Python path changed
  • .github/filters/python-paths.yml is the shared path set both workflows read, so the two if: conditions cannot drift
  • ci.yml carries no pull_request.paths filter, so it always starts and can always report
  • repo_tests/python_required_context_test.py guards the arrangement

So the blocker is not missing code, not a name mismatch, not a swallowed ratchet, and not a per-shard context. ci.yml runs bare pytest — 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-suite aggregator. It does not reproduce: on 2834c6f038e6, python-suite is present and success. It was created at 2026-08-17T22:30:20Z; the audit comment reporting it absent was posted at 22: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:

PR Python touched? ci.yml job shim job higher id (created last)
#14629 yes failure 97253091821 skipped 97250986170 the real verdict
#14805 no skipped 97292839928 success 97293040244 the real verdict

The substantive verdict is created last in both directions, and structurally so: ci.yml's aggregator is needs: [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 with skipped counted as success — it resolves to the real result. That skipped satisfies a required context is directly evidenced on #14805, where code-quality and startup-import-smoke are both required and both skipped.

Swept the 60 most recently merged pull requests to check both claims at scale. Zero had no python-suite check run. The shim's check run was created last in 6 of 60, and all 6 were real=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 into Dev_new_gui on 2026-08-22 with python-suite = failure (shard 12/12 red, aggregator check run 96991538736), five days after the shim landed.

So python-suite is 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.yml runs pre-commit run --all-files and 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.yaml fails rather than silently exempting itself, and an absent or Skipped result fails rather than reading as clean.
  • Wired into the verify-precommit-config job — 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 because python-suite is 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 driving main() rather than a helper.
  • CONTRIBUTING.md — records that pre-commit install and scripts/install-git-hooks.sh are 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 named Set up Python 3.12 while pinning 3.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.txt from the verify-precommit-config job of run 32679083207 (job 97292541874, base e8cb8e7ae), 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.

case expected exit
unmutated CI transcript (Shell SSOT Library Self-Test (#14041) → Passed) pass 0
same line mutated to Failed block 1
result line deleted entirely block 1
result line mutated to Skipped block 1
hook id renamed in the config, output untouched block 1
empty transcript block 1

0 → 1 on 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 e8cb8e7ae the guard reports Passed, 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 to Function Length Check (Issue, and that truncated string is what pre-commit prints. The allowlist resolves id -> name through 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-suite cannot be made to gate from a pull request. Adding a required context is a branch-protection change on Dev_new_gui. Everything up to that line is merged; this is the line.

  • Add the required status context python-suite (exact name, lower-case, hyphenated) to branch protection on Dev_new_gui, taking the list from 10 contexts to 11.
  • After the flip, exercise the acceptance criterion by breaking it rather than by observing green: open a throwaway PR with one deliberately failing Python test and confirm the merge box blocks.

One asymmetry found while verifying this, filed as #14922: the Unit & Integration Tests pair 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 success runs of ci.yml on Dev_new_gui), and the duplicate-name ordering resolves to the real verdict in both directions.

Closes #14878
Refs #14353, #13162

@github-actions

Copy link
Copy Markdown
Contributor

✅ SSOT Configuration Compliance: Passing

🎉 No new hardcoded values of either class — ssot and other both block.

Known backlog in pipeline-scripts/hardcoded_values_baseline.txt is suppressed and tracked in #14371.

@mrveiss
mrveiss merged commit 571bcaa into Dev_new_gui Aug 24, 2026
60 checks passed
@mrveiss
mrveiss deleted the issue-14353-python-suite-gates branch August 24, 2026 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant