feat: workflow updates, pre-commit - #88
Conversation
bryce-lynn-nttd
left a comment
There was a problem hiding this comment.
Solid normalization pass. Verified all third-party SHAs per the provenance habit (setup-uv@08807647 upstream, actions/cache/restore@27d5ce7f upstream, asdf-vm/actions@b7bcd026 upstream); self-reference update-status-check@edddecdce exists in launch-workflows itself (commit "fix: auto-update tool installation (#83)"). The documentation-existence check is preserved — old inline Python lives at scripts/check_documentation_exists.py and runs via a local pre-commit hook gated on (\.github/workflows/reusable-.*\.ya?ml)$. Pre-Commit Checks passing on this very PR is the validation that the new setup runs cleanly across the repo.
Non-blocking — substantive:
-
actions/checkout@v6is tag-pinned, not SHA-pinned, atpull-request-precommit-checks.yml:18. Every other third-party action in this file is SHA-pinned (the repo's documented convention).@v6resolves tode0fac2e45...— pinning to that SHA closes the inconsistency. -
Both
actions/cache/restoresteps lack a correspondingactions/cache/save, so the caches will never populate.actions/cache/restoreis a sub-action that only restores — unlike the top-levelactions/cachewhich auto-saves at post-step. Without a save step, the pre-commit env and asdf tools will rebuild from scratch every run. Two fixes: addactions/cache/savesteps after each install, or switch to the fullactions/cache@<sha>. Pure performance issue, not correctness. -
Branch protection may need updating in lock-step. The labeler workflow job renames
check / Label Pull Request→validate-title / Validate PR Title, and the oldlint.yml'svalidate-documentationis replaced byPre-Commit Checks. Any branch protection rule referencing the old check names will become unsatisfiable after merge. Worth confirming the protection rules get bumped in coordination. -
uv-lockanduv-syncrun unconditionally on everypre-commitstage. Without afiles:filter, they fire regardless of whetherpyproject.toml/uv.lockactually changed. Consider scoping withfiles: ^pyproject\.toml|^uv\.lock$or movinguv-lockto a manual stage for per-commit speed.
Small notes:
- The "manually publish as 1.0.0 after validation" plan is fine — means the auto-released tag will be 0.7.0 (or whatever release-drafter produces) and 1.0.0 lands as a separate manual release. Worth a sentence in the release-on-merge process docs so the next person isn't surprised.
- Whitespace autofix touched many
docs/reusable-*.mdfiles — expected pre-commit-run output.
Generated with Claude Code (Opus 4.7)
ben-vaughan-nttd
left a comment
There was a problem hiding this comment.
Codex PR Review
Note: This pull request review was generated by Codex.
Review Decision
APPROVE
High Findings
- (none)
Medium Findings
- (none)
Low Findings
- Severity: low
Location:.pre-commit-config.yaml:7
Issue: The new pre-commit configuration pins some newly added external hooks below their current upstream releases.
Impact: The PR is intended to bring the repository up to current standards, and the workspace guidance asks for the latest supported external modules.pre-commit/pre-commit-hooksis pinned tov4.5.0while the current release isv6.0.0, andastral-sh/uv-pre-commitis pinned to0.9.7while the current release is0.11.17.
Recommendation: If compatible with the repository supported Python/toolchain range, bump those hook revisions and rerunpre-commit run --all-files; otherwise, document why these older revisions are intentionally held.
Nitpicks
- (none)
Previously Raised Findings Confirmed
- The prior approved review by
bryce-lynn-nttdalready raised several non-blocking observations that are still present in the current PR revision:actions/checkout@v6is tag-pinned, the cache restore steps have no save path, branch protection may need check-name updates, anduv-lock/uv-syncare unscoped. Codex is not claiming these as net-new findings.
Open Questions / Assumptions
- Assumption: The active workspace instruction to use latest supported external modules applies to this review even though the repository itself has no additional instruction files.
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: non-blocking observations from
bryce-lynn-nttdremain present - Net-new blocking findings from Codex: none
- Unresolved blocking findings from other reviewers: none
- Required workflow checks evaluated: yes
- Required workflow failures found: none
- Required workflow failure investigation performed: not needed
- Instruction files discovered: none found in repository
- Instruction files applied: workspace
AGENTS.md - 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:
mise exec -- pre-commit run --all-files;python3 scripts/check_documentation_exists.py
|
Think I may end up just making the pre-commit workflow reusable and adjusting the implementation here accordingly; we seem to be using it more and more so it probably deserves the reusable treatment. In general I agree with most of the feedback here and will follow with another PR addressing most of this stuff before we tag 1.0.0. |
This PR brings this repository up to the latest standards. Some of these were still being settled when this repo was originally created, but now that things are fairly stable I wanted to incorporate them here for consistency's sake.
configuretarget.This whole repo has been run through pre-commit, which found quite a few formatting tweaks to make. This should be much cleaner going forward.
Once I validate this update with some of the downstreams, I intend to create a new release for this repo manually and stabilize this as the
1.0.0release. This will be in addition to 0.7.0 or whatever tag gets generated when this merges.