feat: auto-update incorporates automated pre-commit fixes#82
Conversation
bryce-lynn-nttd
left a comment
There was a problem hiding this comment.
This change is well-scoped and the docs (Mermaid + Behavior section) keep parity with the implementation. Approving with one non-blocking observation about the failure-path coverage.
Verifications
- Control flow traced under GHA's default
bash -eo pipefailshell (and the explicitset -o pipefailalready in scope):pre-commit run --all-files || pre-commit run --all-filescovers the autofix-stabilizes-on-second-pass case correctly. When both runs fail, the script exits mid-step (see non-blocking below). - asdf step order is right: setup → cache/restore →
asdf install→ cache/save (save gated on cache miss). Restore-before-install makes warm runs effectively a no-op. - The YAML description-string reformatting in this diff is exactly the kind of autofix the new behavior is designed to absorb — the PR is dogfooding on its own file. Nice signal.
Non-blocking — failure-path coverage
Before this change, the only pre-commit gate was check-merge-conflict, and any failure routed through pre_commit_passed=false → manual-review PR with diagnostics in the body. After this change, the merge-conflict path still does that, but a non-conflict pre-commit failure (e.g., a hook that legitimately rejects and isn't an autofix, or autofix hooks that can't stabilize in two passes) now exits the run-updates step non-zero before pre_commit_passed is set, so neither the "auto-merge" nor the "requires review" PR-creation step runs at all. The maintainer just sees a red workflow run with no PR.
That's defensible — "rejecting true pre-commit failures" is exactly what the PR description promises — but it's a quieter rejection than the existing manual-review-PR path. If you'd prefer, the failure branch could mirror the merge-conflict block: capture pre-commit run --all-files output, set pre_commit_passed=false, and let the existing manual-review PR creator surface the diagnostics. Either way is fine; flagging the trade-off.
Tiny adjacent note: cmd || cmd is the canonical two-pass autofix idiom and handles essentially all real cases, but cascading autofix hooks (A's output triggers B's autofix on the next pass) can theoretically need three. Not something to design for unless it actually bites.
Generated with Claude Code (Opus 4.7)
The automatic update workflow will now run a full set of pre-commit checks in addition to the preexisting
check-merge-conflictcheck.The current behavior is preserved,
check-merge-conflictfailures trigger a manual PR review. Ifcheck-merge-conflictdoes not fail, the full set of pre-commit checks will be run against the repo, and any changes as a result (formatting, trailing whitespace, etc.) will be incorporated into the automatic update PR (while still rejecting true pre-commit failures). This should allow us to safely mutate e.g. terraform-docs configuration and have those changes automatically reflected in the autogenerated portions of README.md.Because we now need to
asdf installin this workflow, I expect a modest increase in runtime even with caching enabled. The normal case for this workflow is that it runs off-hours on a schedule, so I don't anticipate much real-world impact, but figured it was worth calling out.