Skip to content

Swap pre-commit for prek (#20305)#20314

Open
LeonarddeR wants to merge 29 commits into
nvaccess:masterfrom
LeonarddeR:prek
Open

Swap pre-commit for prek (#20305)#20314
LeonarddeR wants to merge 29 commits into
nvaccess:masterfrom
LeonarddeR:prek

Conversation

@LeonarddeR

@LeonarddeR LeonarddeR commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

Link to issue number:

Closes #20305

Summary of the issue:

The codebase uses pre-commit to run Git hooks at commit time, and relies on the pre-commit.ci hosted app for CI-side autofixing and hook auto-updates. prek is a faster, dependency-free (single Rust binary), drop-in compatible reimplementation that uses uv for hook environments. #20305 asks to swap pre-commit for prek. Since prek is drop-in compatible it reads the existing .pre-commit-config.yaml. This PR switches the local hook runner to prek and reproduces pre-commit.ci's autofix/auto-update behaviour with GitHub Actions. The pre-commit.ci app is kept installed for now (with autofixing disabled) so the beta branch retains hook coverage until it also carries this change.

Description of user facing changes:

None. This is a developer-tooling change only.

Description of developer facing changes:

  • The local Git hook runner is now prek instead of pre-commit. Local hook runs are faster and hook environments are managed by uv. The hook set is unchanged.
  • Hooks stay configured in .pre-commit-config.yaml; prek reads it directly. Hook groups (a prek feature) were added to drive the autofix workflow.
  • Linting, autofixing and hook auto-updates now run as GitHub Actions workflows. The pre-commit.ci app stays installed but its autofixing is disabled (autofix_prs: false) so it no longer pushes commits, and its autoupdate is throttled to quarterly. It is kept only so beta retains coverage until it also carries this change, and will be removed in a follow-up.
  • Developers who previously ran pre-commit install should run uv run prek install -f once to replace the installed Git hook.

Description of development approach:

  • pyproject.toml: replaced pre-commit==4.2.0 with prek==0.4.4 in the lint dependency group, and relocked (uv.lock).
  • .pre-commit-config.yaml: added groups: [autofix] to the fixer hooks (a prek feature) to drive the autofix workflow, set the pre-commit.ci ci: block to autofix_prs: false and autoupdate_schedule: quarterly (GitHub Actions now owns autofix/auto-update), and removed the pre-commit.ci status badge in readme.md.
  • .github/workflows/testAndPublish.yml: the dedicated uv-lock and checkPo steps now invoke uv run prek run ....
  • New .github/workflows/autofix.yml ("Autofix or fail"), one job with two roles:
    • Autofix — on push events (write token), runs the autofix hook group via uvx prek (re-running up to 3× until the tree is stable), commits any fixes as github-actions, and pushes them. Guarded to push events and actor != github-actions[bot] for loop safety. This runs in a contributor's fork when they enable Actions.
    • Lint gate — on every event, a blocking check running all hooks except the 6 heavy/env-bound ones that have dedicated jobs in testAndPublish.yml (checkPo, scons-source, checkPot, unitTest, licenseCheck, pyright). Uses j178/prek-action@v2 so hook environments are cached across runs.
  • New .github/workflows/prekAutoUpdate.yml ("Prek auto-update"): monthly (cron 0 0 1 * *) plus workflow_dispatch. Installs prek standalone, runs prek auto-update --cooldown-days 3 (skips hook releases younger than 3 days to avoid pinning a yanked/broken revision), and opens a PR via peter-evans/create-pull-request. Takes over hook auto-updates from pre-commit.ci (whose own autoupdate is throttled to quarterly here).
  • .github/workflows/add-new-language.yml: dropped its pre-commit install/hook step — the new autofix-or-fail workflow runs on the push of the addLanguage branch it creates, so a separate local lint hook here is redundant.
  • projectDocs/dev/contributing.md and projectDocs/testing/automated.md: updated PR-flow and setup/run instructions for prek and the new workflows, including how the fork autofix flow behaves on a PR.
  • user_docs/en/changes.md: developer changelog entry updated.

Testing strategy:

  • Validated the autofix workflow end-to-end on a fork: a deliberately introduced lint error on a push was auto-fixed, committed, and pushed, with the lint gate then passing on the fixed tree in the same run.
  • Confirmed the fork-PR behaviour: the bot's autofix commit does start a fresh pull_request run on the base repo, but it lands as action_required (held for maintainer "approve and run") because the triggering actor is the Actions bot. A human follow-up push runs the checks without approval. This is documented in contributing.md.

Known issues with pull request:

  • The local licenseCheck hook fails on pre-existing dependency-license matches (e.g. pyphen, requests). This is unrelated to the prek switch — it fails identically under pre-commit — and the hook is in the lint-gate skip list, so CI does not run it.
  • When the fork autofix workflow pushes a fix, the resulting commit's CI run requires a maintainer "approve and run" (bot actor); see the contributing-docs note.
  • Post-merge note: the pre-commit.ci app stays installed so beta keeps hook coverage until it also carries this change. Its autofixing is disabled and its autoupdate throttled to quarterly here, so it stays quiet on master. Uninstalling the app is deferred to a follow-up once beta migrates.

Code Review Checklist:

  • Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons.
  • Security precautions taken.

Replace pre-commit with prek as the local Git hook runner and in the
testAndPublish CI step. prek is a faster, drop-in compatible alternative
that reads the existing .pre-commit-config.yaml unchanged.

- pyproject.toml: pre-commit==4.2.0 -> prek==0.4.4 (lint group); relock
- testAndPublish.yml: uv run pre-commit -> uv run prek
- automated.md: update setup/run docs; add "Switching from pre-commit"
  note (prek install -f to overwrite the legacy hook)
- changes.md: developer changelog entry

The .pre-commit-config.yaml and pre-commit.ci integration are kept as-is.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@LeonarddeR LeonarddeR requested review from a team as code owners June 10, 2026 16:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Updates the repo’s Git hook tooling from pre-commit to prek, adjusting developer docs and CI usage accordingly.

Changes:

  • Replaces pre-commit with prek in the Python lint tool dependencies.
  • Updates developer and testing documentation to describe installing/running hooks via prek.
  • Renames the CI step and switches CI hook invocations from pre-commit to prek.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 7 comments.

File Description
user_docs/en/changes.md Documents the hook-runner migration and one-time reinstall guidance.
pyproject.toml Swaps the lint tool dependency from pre-commit to prek.
projectDocs/testing/automated.md Updates hook setup/run instructions to use prek and adds migration notes.
.github/workflows/testAndPublish.yml Switches the workflow hook execution from pre-commit to prek.

Comment thread projectDocs/testing/automated.md Outdated
Comment thread projectDocs/testing/automated.md Outdated
Comment thread projectDocs/testing/automated.md
Comment thread projectDocs/testing/automated.md Outdated
Comment thread projectDocs/testing/automated.md
Comment thread projectDocs/testing/automated.md
Comment thread .github/workflows/testAndPublish.yml
@LeonarddeR

Copy link
Copy Markdown
Collaborator Author

Just to prove a point, Doing a bare initialization of prek took less then a minute here, whereas pre-commit takes much longer to do the first init.

@seanbudd

Copy link
Copy Markdown
Member

What happens if prek and pre-commit specs become out of sync? I think we should be migrating the CI if we are changing the local env

@LeonarddeR

Copy link
Copy Markdown
Collaborator Author

I think Prek will try to stay a drop-in replacement. There is currently no CI as powerful as Precommit CI that can update pull requests with fixes automatically.

@seanbudd

Copy link
Copy Markdown
Member

can't we do it with github actions?

@LeonarddeR

Copy link
Copy Markdown
Collaborator Author

We can run the hooks in GitHub Actions (there's a prek action), but that only covers checking, not autofixing.

The blocker is pushing fixes back to pull request. Fork-triggered workflows get a read-only GITHUB_TOKEN and no secrets, so Actions can't push to a contributor''s fork branch. Pushing to a fork PR needs a GitHub App, which is probably the main reason why pre-commit.ci exists.

@seanbudd

Copy link
Copy Markdown
Member

I guess we could run checks and autofixing on our branches, and encourage contributors to enable github actions in their fork. We really want to get off pre-commit.ci so we can run jobs faster and on windows. With contributions we can fail their job until they manual fix issues or enable github actions.
I get that we can't automatically fix their issues, but that's also the case currently unless they decide to let NV Access have write access to their fork. Enabling Github actions on your fork is not much more work if you want long-term cloud based auto-linting. I think it would also encourage linting before a PR is opened, when branches are first pushed to.

LeonarddeR and others added 10 commits June 11, 2026 18:20
Convert .pre-commit-config.yaml to prek's native prek.toml via
`prek util yaml-to-toml`. Drop the pre-commit.ci `ci:` block and the
check-pre-commit-ci-config hook (no longer relevant). Tag the eight
auto-fixing hooks with `groups = ["autofix"]` so a CI autofix job can run
only those via `prek run --group autofix`.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replaces pre-commit.ci with native prek: a single job that auto-fixes on
push events and runs a blocking lint gate on every event.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…autofix.yml

Add a step after setup-uv that runs `uv run prek run uv-lock --all-files`,
mirroring the checkPo job in testAndPublish.yml. On pull_request events the
lint gate is the first uv run, so settling the environment here avoids the
spurious hook failure caused by uv regenerating uv.lock mid-run.

Also add a comment noting the hardcoded python-version must be kept in sync
with defaultPythonVersion in testAndPublish.yml.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replaces the monthly pre-commit.ci autoupdate with a GitHub Actions
workflow that runs `prek auto-update` on a monthly schedule (and on
manual dispatch) and opens a pull request with the updated hook
revisions via peter-evans/create-pull-request.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Align the auto-update PR title and commit message with the workflow name
now that the project uses prek instead of pre-commit.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…igration

Update documentation, changelog, and the remaining raw pre-commit usage to
reflect the full migration away from pre-commit.ci to native prek with
GitHub Actions:

- readme.md: remove the obsolete pre-commit.ci status badge.
- projectDocs/dev/contributing.md: replace pre-commit.ci instructions with
  the GitHub Actions lint-gate / fork-autofix model.
- projectDocs/testing/automated.md: rename the hooks section to "Git hooks
  (prek)", state hooks are configured in native prek.toml (not
  .pre-commit-config.yaml), and clarify the uv run vs global invocation styles.
- user_docs/en/changes.md: expand the developer changelog entry to cover
  dropping pre-commit.ci, native prek.toml config, and the new GitHub Actions
  autofix-or-fail and monthly prek auto-update workflows.
- .github/workflows/add-new-language.yml: install prek hooks via uv run prek
  install instead of raw pip install pre-commit.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Use `uvx prek` (an ephemeral, isolated tool run) for both the autofix and
lint-gate steps. `uv run` syncs the project environment and can silently
regenerate uv.lock as a side effect, which both masks a genuinely stale lock
and risks spurious mid-run failures. Running prek via uvx touches no project
state, so the uv-lock hook itself properly gates lock staleness. This also
removes the now-unnecessary "settle the uv environment" step.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The addLanguage branch push triggers the autofix-or-fail workflow (it is a
non-protected branch), which runs the lint gate and autofix. Installing a
local prek Git hook here was redundant and was the last remaining use of
`uv run prek` in CI.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Split the two-sentence list items in the prek invocation-styles list so each
sentence is on its own line, matching the repo's semantic-line-break style.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@LeonarddeR LeonarddeR marked this pull request as draft June 11, 2026 17:47
LeonarddeR and others added 6 commits June 11, 2026 19:52
Run the autofix.yml lint gate via j178/prek-action@v2 instead of
`uvx prek` so prek's hook environments are cached across runs. The
action installs prek standalone, so it never touches uv.lock. The
autofix step keeps using `uvx prek` because it needs the commit/push
logic the action does not provide.

Install prek as a standalone binary in prekAutoUpdate.yml via the
cargo-dist installer instead of uv. auto-update only rewrites hook
revisions in prek.toml, so neither uv nor the project environment is
needed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
LeonarddeR and others added 2 commits June 11, 2026 22:21
The auto-update schedule is an upstream-only maintenance task. Forks enable Actions for the autofix workflow; without this guard they would also run this schedule and open spurious auto-update PRs against their own fork. Restrict the job to github.repository == nvaccess/nvda.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@LeonarddeR LeonarddeR marked this pull request as ready for review June 11, 2026 20:41
@LeonarddeR

Copy link
Copy Markdown
Collaborator Author

@seanbudd I changed the approach to add two extra workflows, one for autofix and one for auto update. The autofix flow also performs basic linting. The bigger checks that were already in the test and publish workflow are still there.
I'm afraid I can't test the auto update flow properly though. I added a guard that it will only run on the nvaccess/nvda repo.

@SaschaCowley SaschaCowley added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Jun 16, 2026
Comment thread .github/workflows/add-new-language.yml
Comment thread .github/workflows/prekAutoUpdate.yml Outdated
Comment thread .github/workflows/prekAutoUpdate.yml Outdated
Comment thread .pre-commit-config.yaml

@seanbudd seanbudd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @LeonarddeR

Comment thread .github/workflows/autofix.yml
Comment thread .github/workflows/autofix.yml Outdated
Comment thread .github/workflows/autofix.yml
on:
schedule:
# Monthly, at midnight on the first day of the month.
- cron: '0 0 1 * *'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SaschaCowley - thoughts on keeping this manual only for now?

Suggested change
- cron: '0 0 1 * *'
# - cron: '0 0 1 * *'

# --cooldown-days skips releases younger than 3 days, avoiding pinning to
# a freshly published (and potentially yanked/broken) hook revision.
- name: Run prek auto-update
run: prek auto-update --cooldown-days 3

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SaschaCowley thoughts on changing this to 7? higher?

Suggested change
run: prek auto-update --cooldown-days 3
run: prek auto-update --cooldown-days 7

Comment thread .pre-commit-config.yaml
@@ -1,30 +1,9 @@
exclude: ^user_docs/(?!en/).+/.+\.md$

# https://pre-commit.ci/

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually we might need to keep CI config for now - or retarget this to beta - otherwise beta is not getting pre-commit hooks coverage.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep pre-commit ci for now. Makes sense to me. Also no problem as long as we stay on .pre-commit.yaml.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I'll see whether we can otout of precommit ci. then every branch that has the changes will opt out automatically.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that how it works? I thought it just uses the rules from master for all branches?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is actually not completely clear what the config does. Documentation reads re the target branch for update pull requests:

this configuration option and other autoupdate settings will be read from the .pre-commit-config.yaml in the default branch of the repository.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that sounds like .pre-commit-config.yaml on master is used for all branches

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but only for the ci config, not for running the actual hooks. I also think that skip is actually read on the branch, otherwise you aren't able to test it in pull requests.

Comment thread .github/workflows/autofix.yml

@seanbudd seanbudd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @LeonarddeR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

swap pre-commit for prek

4 participants