Re-verify before amending; make the unpushed check fail closed

[sid2687]

Fixes #6.

The problem

On a FAIL, the unpushed check runs once, but the message it produces is acted on much later: review latency, plus however long the agent takes to write the fix. Anything that pushes on its own schedule (a sync daemon, a parallel session, an IDE auto-push) can land inside that window. By then, "it has not been pushed, so amending is safe" is an instruction to rewrite published history.

Seen twice in one day across two repositories. In one, the commit landed at 12:42:50 and a daemon pushed it at 12:47:17: the advice was true when written and false 4.5 minutes later.

Worth ruling out, because it looks like one: this is not a stale-refs bug. The push came from the same clone, so the remote-tracking ref did update. A git fetch inside the hook would not help. Nothing evaluated at review time can stay true until the fix lands.

The change

The amend branch now reports when the check was true and hands the agent a re-check to run first. The already-pushed branch is untouched, since it was never wrong.

Why the re-check looks over-built

Because "no output" is the trap. An empty result is exactly what means "safe to amend", so every way of producing no output has to be told apart from a genuine empty answer. The obvious re-check reintroduces the original bug one level down:

git fetch --quiet && git branch -r --contains <sha>

&& short-circuits, so a failed fetch prints nothing, and that reads as permission. A failed lookup does the same, since branch -r --contains prints nothing on an unknown or ambiguous sha.

So each step has to succeed before the emptiness test is reached:

git -C <root> fetch --all --quiet \
  && amend_check=$(git -C <root> branch -r --contains <full-sha>) \
  && [ -z "$amend_check" ] \
  && echo AMEND-OK || echo DO-NOT-AMEND

• Unreachable remote, failed lookup, and already-pushed commit all print DO-NOT-AMEND.
• Full sha rather than an abbreviation, which can be ambiguous in a large repository.
• --all, since a bare git fetch refreshes only the default remote and could miss a push to another.
• <root> is shell-quoted. The agent runs this verbatim, and unquoted it was a syntax error on any repository whose path contains a space, which is ordinary on macOS.

Each of those last three was a separate fail-open, and each was found by this plugin reviewing the commit that fixed the previous one. I have left the intermediate steps out of the branch, but they are worth noting: the plugin caught three layers of the same mistake in its own patch.

Tests

Five, in tests/post-commit-review.bats:

• the advice carries the re-check, and the command survives jq encoding unexpanded
• the full sha is passed, not an abbreviation
• the emitted command is extracted and run: unpushed, pushed, and with the remote deleted
• the same, against a sha that is not in the repository
• the same, from a repository path containing a space and an apostrophe

I checked that the last three each fail without their corresponding fix, rather than passing either way.

npx --yes bats tests is 86/86. shellcheck (0.11.0) is clean on all three hook scripts. No new dependencies; bash, git, and jq only.

Notes

I bumped plugin.json to 1.7.1 and added a CHANGELOG section, following how 1.6.0 and 1.7.0 landed. Happy to drop the bump if you would rather batch it into a larger release, and equally happy to trim the comment block if it reads as too much for the size of the diff.

Thanks for the plugin. It has been genuinely useful, including at my expense here.

[sid2687]

Heads up that CI is sitting at action_required on this one. That is the first-time-contributor gate on fork PRs, so it needs an "Approve and run" from you before anything will report. No rush from my side, just flagging it since the PR otherwise looks like it has no checks rather than pending ones.

For what it is worth locally: npx --yes bats tests is 86/86, and shellcheck 0.11.0 is clean on all three hook scripts.

[andreidavid]

This is one of the best bug reports and fixes this project has received — and it earned a hard look, because it changes what the hook tells the agent to execute. It passed that look completely.

Three things deserve to be called out. First, the issue itself is a model report: the reflog timeline, the explicit ruling-out of the stale-refs explanation, and the asymmetric-cost framing ("a wrong amend rewrites shared history, a wrong new commit costs one extra commit") made the review easy because the thinking was already done. Second, the fail-closed analysis — "no output is the trap" — is exactly right, and the discipline of making every step succeed before the emptiness test is reached shows in all three layers you caught. Third, the tests actually execute the emitted command against real unpushed/pushed/deleted-remote/bogus-sha/nasty-path states rather than just grepping for it. That's rarer than it should be.

A confession from our side: two releases landed underneath you while this sat (that's why it now conflicts), and one of them — #9 — reintroduced your exact point-in-time pattern in its new INCONCLUSIVE retry guidance. Your fix arrives just in time to be applied there too.

Since the conflicts are our doing, we'll take it from here: merging via a merge commit so the authorship and the merge badge stay yours, resolving the version to 1.8.1, and extending your re-check to the INCONCLUSIVE arm in a follow-up on our side. Credit in the CHANGELOG with links to both this PR and #6.

Thank you — including for the grace of getting bitten by our advice twice in one day and responding with this instead of an angry issue.