fix(hooks): a graphify skip must not terminate the whole git hook (#2986) - #3108
fix(hooks): a graphify skip must not terminate the whole git hook (#2986)#3108abhay-codes07 wants to merge 1 commit into
Conversation
…aphify-Labs#2986) The generated post-commit / post-checkout block is appended to whatever hook a repo already has, and other tools chain their logic after it. Every skip condition in the block was a bare `exit 0`, which in a flat sh script ends the ENTIRE hook process — so anything after graphify's end marker was silently dropped on every root commit (HEAD~1 does not exist), every rebase/merge/cherry-pick, every linked worktree, every GRAPHIFY_SKIP_HOOK=1 and whenever no graphify-capable python could be found. Both bodies now run inside a subshell `( ... )`, so an `exit` ends only graphify's section: no rewriting of the seventeen skip sites, no `return`-outside-a-function hazard for the _PYTHON_DETECT fragment that tests run standalone, the detached rebuild launch unaffected, and the hook's own exit status 0 as before. The markers stay outside the subshell so reinstall/uninstall/status keep finding the block.
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
Wraps the generated post-commit and post-checkout hook bodies in a subshell ( ... ) so graphify's internal exit 0 skip paths (root commit, rebase/merge, linked worktree, GRAPHIFY_SKIP_HOOK=1) end only its own section instead of the entire hook, letting anything chained after the end marker keep running (#2986). The start/end markers stay outside the subshell so install/uninstall/status still find and update the block in place.
No blocking issues surfaced. 3 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 362 functions depend on the 205 functions this change touches.
Health — this change adds coupling hotspots:
- new:
install()— 37 callers, 7 callees - new:
dispatch_command()— 2 callers, 122 callees - new:
dispatch_install_cli()— 2 callers, 31 callees - new:
status()— 8 callers, 6 callees - new:
uninstall()— 9 callers, 5 callees - new:
uninstall_all()— 2 callers, 13 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees
Verification — 362 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 212 function(s) in the blast radius were not formally verified this run
· 7 more finding(s) on lines outside this diff (see the check run).
Closes #2986.
The problem
The generated
post-commit/post-checkoutblock is appended to whatever hook a repo already has, and other tools chain their own logic after graphify's end marker. Every skip condition in the block was a bareexit 0— seventeen of them in 0.9.48 — which in a flatshscript ends the entire hook process, not graphify's section. Anything after the marker was silently dropped:HEAD~1does not exist, soCHANGEDis empty);GRAPHIFY_SKIP_HOOK=1;Each
exit 0is right as an intent ("graphify should not run here") and wrong as a mechanism in a shared hook file.The change
Both hook bodies now run inside a subshell:
An
exitinside the subshell ends only graphify's section. This keeps the seventeen skip sites byte-for-byte as they were, avoids rewriting them toreturn(which would break the_PYTHON_DETECTfragment tests run standalone —returnoutside a function is undefined in POSIX sh), leaves the detached rebuild launch unaffected, and preserves the hook's own exit status of 0. The markers stay outside the subshell so reinstall / uninstall /hook statuskeep finding the block, and the in-place update path still reports "already installed" for an unchanged block.One existing test slices the checkout script's prefix and runs it under
sh -c; it now closes the subshell after its sentinel.Tests
tests/test_hook_chain_survives_skip.py— 7 tests that install the real hooks into a real git repo, appendecho AFTER_GRAPHIFYafter the block, and run them undersh: aGRAPHIFY_SKIP_HOOK=1skip, a rebase in progress, an empty diff (the root-commit shape), a non-branch checkout, a skipped checkout hook, a pre-existing hook before the block, and reinstall idempotence. Each asserts the trailer ran, the exit status is 0, and (for the skip case) that no rebuild was launched. With the subshell reverted, 5 of 7 fail.tests/test_hooks.pyis otherwise unchanged; the full suite matches thev8baseline.