chore: fmt hook — skip delete pushes; gate auto-commit on HEAD-in-push

[briansrls]

Summary

Follow-up to #503. Tightens the pre-push hook so it doesn't auto-commit to an unrelated branch when the push is a deletion or a cross-branch push.

What was wrong

The previous hook checked fmt on every push regardless of what was being pushed. Observed after #497 merge: `git push origin --delete docs/db-14-substrate-external-primitives` fired the hook from a worktree on an unrelated branch; fmt drift on that branch got auto-committed as `chore: apply cargo fmt` on the worktree's branch — even though no content was being pushed, and the push target was just a remote-ref deletion.

Harmless in that case (the branch was local-only) but surfaces a real UX wart: auto-commit should only happen when the commit will land on a ref being pushed.

New decision table

Reading stdin per git's pre-push contract (one line per ref being pushed, format `<local_ref> <local_sha> <remote_ref> <remote_sha>`, zero local SHA = deletion):

Push type	Behavior
Delete-only (all refs are deletions)	Skip entirely — no content to fmt-check
Push includes current HEAD	Existing behavior — clean-tree check + cargo fmt + chore commit on top of HEAD
Push excludes current HEAD	fmt-check only — no auto-commit (would land on a ref that isn't reaching the remote); fail with actionable message

Test plan

• Delete push: `git push origin --delete some-branch` from a worktree with fmt drift → hook exits 0, no commit.
• HEAD push with drift: existing behavior preserved (auto-fix + chore commit + push continues).
• HEAD push clean: existing behavior (exit 0, no noise).
• Cross-branch push with drift (push origin other:other while on main with fmt drift): hook reports drift, does NOT auto-commit to main, fails with actionable message.

Already installed

I've also updated my local `.git/hooks/pre-push` to the new version (the `.git/hooks/` install survives across branch changes since it's outside version control). When this merges, re-running `scripts/install-hooks.sh` picks up the new `.githooks/pre-push` automatically for anyone using the `core.hooksPath .githooks` install pattern.

🤖 Generated with Claude Code

[briansrls]

ChatGPT Review

Principle audit.

Fail-closed. This is implementation-local hook logic, not substrate, and the new control flow is mostly fail-closed in the modeling-discipline sense: delete-only pushes exit intentionally; dirty trees only block when an auto-commit could accidentally sweep them up; and fmt drift outside the HEAD-in-push case now fails with an explicit, actionable message instead of silently creating a commit that will not be pushed. That fits the thesis’s “validate the causal chain before acting” lens. chatgpt-review-1ba9a301-24da-4f…

chatgpt-review-c1f4a072-3f21-45…

Illegal states unrepresentable. No substrate or cross-pass data model changed here, so there is no new illegal-state surface to bank debt into. For shell, the pushes_content / current_head_in_push split is a reasonable encoding of the three intended push classes.

Facts flow forward. This is the one place I have a real comment. The diff is an improvement because it finally consumes push-shape data from Git’s pre-push stdin instead of treating every push as “push current HEAD.” But at .githooks/pre-push:27-33 it still drops local_ref and decides safety only from local_sha == current_head. Git gives the hook both the local ref and the local object name, and it preserves non-branch sources in local-ref; that means the hook is still using only part of the available fact. NON-BLOCKING: I think that leaves an edge case where “OID equals HEAD” is true but “a new commit on top of HEAD will ride along on the pushed ref” is false. Git SCM+1

Coproduct dissolution. No new Rust enum or substrate coproduct appears in this diff, so the dissolution ledger/trigger requirement does not apply. This is ordinary implementation code.

Single authority. Better than before: the push description now comes from Git’s stdin, which is the right authority, rather than from an implicit “current branch push” assumption. The only remaining weakness is the same one above: the hook still does not consume the full authoritative tuple when deciding whether auto-commit is safe.

API-level enforcement. The new policy is encoded in control flow, not left as commentary: delete-only pushes skip, HEAD-not-in-push refuses auto-commit, HEAD-in-push keeps the old auto-fix path. That is the right shape for implementation code.

Design question.

Is the safety gate really “some pushed object ID equals HEAD,” or is it “the pushed source is a movable ref currently at HEAD”? The latter seems closer to the comment’s promise that the chore commit will “land on the ref being pushed.” Git’s pre-push contract exposes both local-ref and local-object-name, and Git explicitly distinguishes heads from tags because commits move heads but do not update tags; if that stronger guarantee is the real design goal, local_ref needs to participate in the gate. Git SCM+1

Verdict.

APPROVE_WITH_COMMENTS. This is a good, implementation-local tightening of the hook: it fixes two real misfires without introducing scaffolding or parallel authority. My only comment is the ref-identity edge case above, which I’d treat as NON-BLOCKING unless this repo often pushes tags or explicit refspec/object sources through the same hook.

LOOP HEALTH: converging — this round consumes more of Git’s real push-shape facts and removes two concrete hook misbehaviors, with only a narrow ref-identity nuance still left unmodeled.

---

View conversation

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 35d67a7d3d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Gate auto-commit on pushed ref, not only matching SHA

current_head_in_push is flipped when local_sha == current_head, but that is also true when pushing a different ref that happens to point at the same commit (for example, pushing feature that shares the current tip, or a lightweight tag at HEAD). In that case the hook takes the auto-commit path and creates chore: apply cargo fmt on the checked-out branch even though that new commit is not in the pushed refset, which reintroduces the unintended “commit on unrelated branch” behavior this change is meant to prevent.

Useful? React with 👍 / 👎.

[briansrls]

codex · gpt-5.4 · 35d67a7d

⚠️ Review (blocking: 1, non-blocking: 1+/0-)

BLOCKING (1)

Root Cause

• .githooks/pre-push The design assumes a pre-push hook can mutate the ref being pushed, but ref selection is already fixed when the hook executes -> make this hook check-only/fail-closed, or move auto-fix+commit earlier than git push ref resolution.

Non-blocking — Strengths

• .githooks/pre-push The delete-only short-circuit and the explicit cross-branch failure message do address the unrelated-branch auto-commit case that motivated this PR.

⚠️ The delete-push guard is fine, but the new HEAD-in-push gate still rests on a false pre-push assumption and will report a successful auto-fix that never reaches the remote.

[briansrls]

BLOCKING: The "HEAD is being pushed -> safe to auto-commit" branch is incorrect because Git snapshots the refs-to-push before pre-push runs, so the new fmt commit is not sent and the hook succeeds with unresolved drift (Fail-Closed).

[briansrls]

codex · gpt-5.4 · 35d67a7d

⚠️ Review (blocking: 1, non-blocking: 1+/0-)

BLOCKING (1)

Root Cause

• .githooks/pre-push The design assumes a pre-push hook can mutate the ref being pushed, but ref selection is already fixed when the hook executes -> make this hook check-only/fail-closed, or move auto-fix+commit earlier than git push ref resolution.

Non-blocking — Strengths

• .githooks/pre-push The delete-only short-circuit and the explicit cross-branch failure message do address the unrelated-branch auto-commit case that motivated this PR.

⚠️ The delete-push guard is fine, but the new HEAD-in-push gate still rests on a false pre-push assumption and will report a successful auto-fix that never reaches the remote.

[briansrls]

Addressed BLOCKING (commit 4267cf0)

Reviewer correctly caught that `git push` builds the push pack from resolved local SHAs before calling the pre-push hook. A commit created inside the hook advances the local ref, but git has already captured the pre-hook SHA in the pack — the new commit stays local and doesn't reach the remote. The "[pre-push] committed fmt fixes — push continuing with the new commit" message was lying.

Fix

Keep the auto-fix + auto-commit convenience (so the fmt fix is already prepared as a chore commit — user doesn't have to type `cargo fmt --all && git add -u && git commit`), but exit 1 after the commit to abort the push. User re-runs `git push` and the chore commit ships alongside the original work.

New decision table

Push type	Behavior
Delete-only	Skip entirely
Push includes HEAD, fmt clean	exit 0, push proceeds (unchanged)
Push includes HEAD, fmt drift	auto-fix + auto-commit + abort push; developer re-runs
Push excludes HEAD, fmt clean	exit 0, push proceeds (unchanged)
Push excludes HEAD, fmt drift	fail with actionable message (unchanged)

New abort message (honest about the git contract)

```
[pre-push] fmt fixes committed locally as 'chore: apply cargo fmt'.
[pre-push] Git has already built the push pack with the pre-hook SHA, so
[pre-push] the new commit would NOT reach the remote on this push.

Run 'git push' again to push the fmt commit alongside your original work.
```

One other small fix

The `cargo fmt --check says drift but --all produces no diff` edge case previously exited 0 (swallowing the anomaly). Now fails loud with a message — real bugs should surface.

Test plan

• Fmt-clean push: exit 0, push proceeds
• Fmt-drift push on HEAD: auto-fix + chore commit lands locally; push aborts with "re-run git push" message; second `git push` ships the new commit
• Fmt-drift cross-branch push: check-only fail (unchanged)
• Delete push: skip (unchanged)

Thanks for catching this — the hook was silently wrong about what it was shipping.

[briansrls]

ChatGPT Review

Principle audit.

Fail-closed. This is materially better than the previous version. The main Codex issue is fixed: on the HEAD path the hook now auto-fixes, commits locally, and aborts, rather than pretending the new commit rides the in-flight push. Delete-only pushes are also handled explicitly, which matches Git’s pre-push input contract for delete refs. My one NON-BLOCKING concern is that the success path still is not tied to the ref(s) being pushed: .githooks/pre-push:35-43 parses the pushed refs, but .githooks/pre-push:59-60 validates only the checked-out tree. Git’s pre-push interface is ref-oriented (<local ref> <local sha> <remote ref> <remote sha>), so a cross-ref push can still be accepted or rejected based on the wrong tree. Git SCM+1

Illegal states unrepresentable. For a Bash hook, this is fine. pushes_content and current_head_in_push are each single-purpose, and the "--check reported drift but fmt produced no diff" case at .githooks/pre-push:86-91 now fails loudly instead of silently exiting clean.

Facts flow forward. This is where I still see the real design miss. The hook receives the authoritative pushed-ref facts on stdin at .githooks/pre-push:35-43, but then reduces them to a boolean and throws away the identity that the later decision actually needs. The consumer at .githooks/pre-push:59-75 no longer knows which ref it is judging. That is a classic lost-fact boundary: the script knows “what is being pushed,” but the actual fmt gate only knows “something somewhere matched HEAD.” NON-BLOCKING, but real.

Coproduct dissolution. No substrate enum work here, and the branching is implementation-local shell logic. I do not see a coproduct concern.

Single-authority metadata. Same root cause in a sharper form: current_head_in_push currently means “some pushed SHA equals HEAD” (.githooks/pre-push:29-41), but the question the auto-commit path really needs is “will git commit advance the ref being pushed?” Those are not the same authority. If branch A is checked out and branch B or a tag also points at the same commit, the auto-commit path can still fire even though the push does not include the ref that the commit would move. Git’s docs explicitly preserve both the local ref name and local SHA on stdin, and this script needs both. Git SCM+1

API-level enforcement. Reasonably good for a hook, but this is still enforced by convention more than mechanism. The comments and docs say cross-branch pushes skip the auto-commit path, but the implementation only approximates that through SHA equality. A tighter boundary would either make this hook explicitly HEAD-branch-only, or inspect the pushed local_ref/local_sha trees directly so the wrong-tree case cannot happen.

Design question.

Is this hook supposed to validate the checked-out worktree, or the refs named by pre-push stdin? That is the deepest structural question here. Right now it is a hybrid: it uses stdin only to derive current_head_in_push, then uses the checkout for the real fmt decision. If the intent is HEAD-only, I would make non-HEAD pushes an explicit skip/error up front. If the intent is “validate what will be pushed,” then the hook needs to check the pushed ref identity/tree directly instead of treating SHA equality with HEAD as sufficient.

Verdict.

APPROVE_WITH_COMMENTS. The core false assumption from the prior round is fixed, and the new diagnostics are honest and helpful. My only real concern is NON-BLOCKING: the cross-ref / alias-ref path still collapses “pushed ref” into “current HEAD” too early, so the hook can judge or auto-commit against the wrong authority.

LOOP HEALTH: converging — this round fixes the original “pre-push can mutate the in-flight pack” mistake, and the remaining issue is a narrower authority mismatch rather than a new spread of debt.

---

View conversation

[briansrls]

Meta-review in progress... (view conversation)

Loop-health check: is this review cycle making forward progress, or shifting debt? Posts in ~5-15 minutes.

[briansrls]

Addressed round-2 NON-BLOCKING — ref name, not SHA, is the authority (commit 7d1c057)

Reviewer correctly caught: matching pushed refs to HEAD by SHA equality alone false-positives on aliased refs. Two concrete scenarios the old version got wrong:

1. Tag pushes aliased to HEAD's commit. `local_ref = refs/tags/`, `local_sha = HEAD". Old hook treated as "HEAD is being pushed" → auto-commit onto HEAD when the push target was a tag.
2. Branch B aliased to branch A (checked out). Push of B has `local_ref = refs/heads/B`, not `refs/heads/A`. SHA matched HEAD's SHA → old hook thought A was being pushed.

In both cases the auto-commit would have landed on the checked-out branch while the push carried a different ref — exactly the cross-ref bug the reviewer described.

Fix

Resolve HEAD's full ref via `git symbolic-ref HEAD` (returns `refs/heads/`, empty on detached HEAD) and match pushed `local_ref` against that. Ref identity is preserved through the decision path; reduction to boolean happens only after the authoritative comparison.

Also renamed the variable `current_head_in_push` → `head_branch_in_push` so the name matches what the check does: "the current branch's ref is in the push list," not "some pushed SHA equals HEAD."

Detached HEAD explicitly handled

`git symbolic-ref HEAD` fails (empty result) in detached state. Auto-commit can't fire (no branch to commit onto). The fmt-drift branch now splits:

```
If HEAD is detached:
"fmt drift detected, but HEAD is detached — no current branch to auto-commit onto."
→ attach HEAD to a branch, or run cargo fmt + commit explicitly

If HEAD is on branch but that branch isn't being pushed:
"fmt drift detected, but the push doesn't include the current branch ($head_ref)."
→ switch branches and re-push, or commit the fmt fix explicitly
```

Test plan

• Push current branch with drift → auto-fix + commit + abort (unchanged)
• Push current branch clean → exit 0 (unchanged)
• Push tag pointing at HEAD with drift → cross-ref fail (previously: wrong auto-commit)
• Push other branch with same SHA as HEAD → cross-ref fail (previously: wrong auto-commit)
• Push from detached HEAD with drift → explicit detached-state message
• Delete push → skip (unchanged)

Authority-flow observation the reviewer made

Git's pre-push stdin is ref-oriented; the hook was collapsing that authority too early by reducing to a boolean via SHA matching. The fix preserves the ref-name identity through the decision path. That's the kind of "facts flow forward" issue we've been disciplined about in substrate design; nice to see it apply to shell-script hooks too.