Skip to content

feat: auto-update incorporates automated pre-commit fixes#82

Merged
chris11-taylor-nttd merged 1 commit into
mainfrom
auto-update-pre-commit-behavior
May 14, 2026
Merged

feat: auto-update incorporates automated pre-commit fixes#82
chris11-taylor-nttd merged 1 commit into
mainfrom
auto-update-pre-commit-behavior

Conversation

@chris11-taylor-nttd
Copy link
Copy Markdown
Contributor

The automatic update workflow will now run a full set of pre-commit checks in addition to the preexisting check-merge-conflict check.

The current behavior is preserved, check-merge-conflict failures trigger a manual PR review. If check-merge-conflict does 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 install in 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.

Copy link
Copy Markdown
Contributor

@bryce-lynn-nttd bryce-lynn-nttd left a comment

Choose a reason for hiding this comment

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

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 pipefail shell (and the explicit set -o pipefail already in scope): pre-commit run --all-files || pre-commit run --all-files covers 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)

@chris11-taylor-nttd chris11-taylor-nttd merged commit 1dd2a82 into main May 14, 2026
3 checks passed
@chris11-taylor-nttd chris11-taylor-nttd deleted the auto-update-pre-commit-behavior branch May 14, 2026 15:25
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.

3 participants