Skip to content

fix: pre-commit tweaks - #91

Open
chris11-taylor-nttd wants to merge 3 commits into
mainfrom
minor-tweaks-before-release
Open

fix: pre-commit tweaks#91
chris11-taylor-nttd wants to merge 3 commits into
mainfrom
minor-tweaks-before-release

Conversation

@chris11-taylor-nttd

Copy link
Copy Markdown
Contributor

Other minor fixes from #88 not covered in #89 and #90:

  • Bumped uv version to latest in .tool-versions
  • Bumped pre-commit-hooks and uv-pre-commit versions in .pre-commit-config.yaml
  • Implemented file specifier for uv pre-commit actions to keep sync from running on commit unless one of the relevant files has changed.

@github-actions github-actions Bot added the fix label May 29, 2026

@bryce-lynn-nttd bryce-lynn-nttd 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.

Clean follow-up to PR #88's normalization. Directly addresses my prior non-blocking note about uv-sync/uv-lock running unconditionally on every commit — now scoped with files: ^pyproject\.toml|^uv\.lock$ filters.

Verifications:

  • Both uv-lock and uv-sync (pre-commit stage) gated on actual config changes via the files: filter.
  • uv-sync correctly split into two hook entries: pre-commit stage with the file filter, and post-checkout/post-merge/post-rewrite stages without filter (those are user-initiated events with no "files changed" semantic, so unconditional run is right).
  • Version bumps aligned: uv 0.8.16 → 0.11.17 in .tool-versions matches uv-pre-commit 0.9.7 → 0.11.17 in .pre-commit-config.yaml.
  • CI's Pre-Commit Checks passing on this PR confirms the pre-commit-hooks v4.5.0 → v6.0.0 (skipping v5) lands clean for the hooks actually configured here.
  • Title-driven fix:fix label applied correctly under the new scheme PR #88 introduces.

Non-blocking:

  • pre-commit-hooks v4.5.0 → v6.0.0 skips a major version. v5.x.x had a few hook signature changes (mostly around argument defaults). For the hooks actually used here, CI confirms it's clean — purely informational.

Generated with Claude Code (Opus 4.7)

@ben-vaughan-nttd ben-vaughan-nttd left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Codex PR Review

Note: This pull request review was generated by Codex.

Review Decision

REQUEST_CHANGES

High Findings

  • (none)

Medium Findings

  • Severity: medium
    Location: .pre-commit-config.yaml:40
    Issue: The pre-commit-stage uv-sync hook still runs for unrelated file changes despite the new files: filter.
    Impact: The PR description says this change keeps uv-sync from running on commit unless relevant files changed, but uv-sync from uv-pre-commit declares always_run: true in its hook manifest. Because this override does not set always_run: false, a README-only pre-commit run still executed uv-sync, preserving the unnecessary commit-time sync behavior this PR is meant to remove.
    Recommendation: Add always_run: false to the pre-commit-stage uv-sync override, and consider anchoring the file regex as ^(pyproject\.toml|uv\.lock)$ for clarity. Keep the post-checkout/post-merge/post-rewrite uv-sync hook unconditional.

Low Findings

  • (none)

Nitpicks

  • (none)

Previously Raised Findings Confirmed

  • The prior reviewer’s informational note about pre-commit-hooks jumping from v4.5.0 to v6.0.0 still applies. Codex is not claiming this as a new finding.

Open Questions / Assumptions

  • (none)

Required Workflow Status

  • Required workflows overall: pass
  • Failing required workflows: none
  • Pending required workflows: none

Validation Notes

  • Local checkout used: yes
  • Existing PR comments/reviews checked: yes
  • Existing findings confirmed but not claimed as new: prior major-version-jump note remains informational
  • Net-new blocking findings from Codex: pre-commit-stage uv-sync still runs for unrelated files
  • Unresolved blocking findings from other reviewers: none observed
  • Required workflow checks evaluated: yes
  • Required workflow failures found: none
  • Required workflow failure investigation performed: not needed
  • Instruction files discovered: none found
  • Instruction files applied: workspace-level review guidance
  • Instruction files skipped as out-of-scope: none
  • User clarification requested for ambiguous scope: no
  • Older duplicate Codex blocking review dismissed: no
  • Checks/tests executed: pre-commit validate-config; pre-commit run --all-files; pre-commit run uv-lock --files pyproject.toml; pre-commit run uv-sync --hook-stage pre-commit --files pyproject.toml; pre-commit run uv-lock --files README.md; pre-commit run uv-sync --hook-stage pre-commit --files README.md; pre-commit run --files README.md; git diff --check

@chris11-taylor-nttd

Copy link
Copy Markdown
Contributor Author

Good catch. Was able to validate this by making and then rolling back a local commit and confirming that uv-sync doesn't fire:

$ touch foo.bar.md
$ git add foo.bar.md 
$ git commit -m "test";
trim trailing whitespace.................................................Passed
check for case conflicts.................................................Passed
check that executables have shebangs.................(no files to check)Skipped
check json...........................................(no files to check)Skipped
check for merge conflicts................................................Passed
check that scripts with shebangs are executable..........................Passed
check yaml...........................................(no files to check)Skipped
fix end of files.........................................................Passed
mixed line ending........................................................Passed
Detect secrets...........................................................Passed
uv-lock..............................................(no files to check)Skipped
uv-sync..............................................(no files to check)Skipped
Documentation must exist for reusable workflows......(no files to check)Skipped
[minor-tweaks-before-release 3fcc9ac] test
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 foo.bar.md

@ben-vaughan-nttd ben-vaughan-nttd left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Codex PR Review

Note: This pull request review was generated by Codex.

Review Decision

APPROVE

High Findings

  • (none)

Medium Findings

  • (none)

Low Findings

  • (none)

Nitpicks

  • (none)

Previously Raised Findings Confirmed

  • Codex’s prior uv-sync finding is resolved in the current revision. The pre-commit-stage uv-sync override now sets always_run: false, and README-only validation skips both uv-lock and uv-sync.
  • The prior reviewer’s informational note about pre-commit-hooks jumping from v4.5.0 to v6.0.0 still applies. Codex is not claiming this as a new finding.

Open Questions / Assumptions

  • (none)

Required Workflow Status

  • Required workflows overall: pass
  • Failing required workflows: none
  • Pending required workflows: none

Validation Notes

  • Local checkout used: yes
  • Existing PR comments/reviews checked: yes
  • Existing findings confirmed but not claimed as new: prior uv-sync blocker verified resolved; prior major-version-jump note remains informational
  • Net-new blocking findings from Codex: none
  • Unresolved blocking findings from other reviewers: none observed
  • Required workflow checks evaluated: yes
  • Required workflow failures found: none
  • Required workflow failure investigation performed: not needed
  • Instruction files discovered: none found
  • Instruction files applied: workspace-level review guidance
  • Instruction files skipped as out-of-scope: none
  • User clarification requested for ambiguous scope: no
  • Older duplicate Codex blocking review dismissed: no
  • Checks/tests executed: pre-commit validate-config; pre-commit run --all-files; pre-commit run --files README.md; pre-commit run uv-sync --hook-stage pre-commit --files README.md -v; pre-commit run uv-lock --files pyproject.toml; pre-commit run uv-sync --hook-stage pre-commit --files pyproject.toml; git diff --check

@bryce-lynn-nttd bryce-lynn-nttd 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.

This supersedes my earlier approval (dismissed on the new commit), which was wrong: I claimed the files: filter alone gated the pre-commit-stage uv-sync, but uv-sync ships always_run: true in its manifest, so the filter was inert. Codex's CHANGES_REQUESTED was a legitimate catch. HEAD (de503aa) resolves it.

Verifications at HEAD:

  • Pre-commit-stage uv-sync now sets always_run: false alongside files: ^pyproject\.toml|^uv\.lock$ — both are required; the filter does nothing without disabling always_run.
  • Post-checkout/post-merge/post-rewrite uv-sync stays unconditional, which is correct (user-initiated events, no "files changed" semantic).
  • uv-lock (pre-commit) uses only the files: filter and skips correctly — confirmed empirically by Chris's manual commit test and Codex's README-only runs (both uv-lock and uv-sync Skipped).
  • Version alignment: .tool-versions uv 0.8.16 → 0.11.17 matches uv-pre-commit rev 0.11.17.
  • CI green at HEAD: Pre-Commit Checks, Validate PR Title, Label PR all SUCCESS.

Non-blocking (optional):

  • Regex ^pyproject\.toml|^uv\.lock$ parses as ^pyproject\.toml OR ^uv\.lock$; the first alternative isn't end-anchored. ^(pyproject\.toml|uv\.lock)$ is clearer. Harmless in practice.
  • pre-commit-hooks v4.5.0 → v6.0.0 skips v5; CI confirms the configured hooks land clean. Informational.

Generated with Claude Code (Opus 4.8)

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants