fix(hooks): hooks that never run, and the guards that cannot see them (#14909, #14891, #14884)

[mrveiss]

Closes #14909, #14891, #14884

Three findings, one root cause: hook files are tracked without the executable bit, and core.fileMode=false hides that from every local check. Fixing the modes without widening the guards that should have caught them would leave the class open, so all three land together.

Thinking Path

The three issues describe different symptoms — a dangling symlink, a *.sh glob, a FileNotFoundError — but they are one shape: a check that is absent reads exactly like a check that passed.

core.fileMode=false is the mechanism behind the first. A plain chmod +x makes the working file executable and never reaches the index; git status stays clean, the local hook runs, and every other clone gets 100644 — which git skips silently. So the fix has to be git update-index --chmod=+x, and the guard has to read git ls-files -s. A guard using os.access or stat would have reported the locally-patched working file and passed on a repository that was still broken, which is the same failure one level up.

Widening #14891's walk was not a one-line glob change. test_each_failure_block_actually_exits requires exit [1-9], but pre-commit-warn-untracked's documented contract is that it must never block a commit, so its block correctly ends in exit 0. A filename allowlist would have made the guard's reach a maintained list whose failure mode is silence. Instead the third category is earned from the block itself — a lib-source-contract: non-blocking marker, a message on stderr, an explicit exit 0 — and bounded above so it cannot become the escape hatch that turns the whole guard green.

The reach floors mattered more than the glob. _SCANNED > 120, len(_SITES) >= 60 and >= 55 distinct scripts were all satisfied by the .sh population alone, so the guard reported healthy coverage over its own blind spot. Every floor is re-derived at the widened count, plus a new one that only an extensionless script can satisfy — so a regression to rglob("*.sh") fails on something the .sh files cannot rescue.

One correction to #14884. The issue records both suites failing with FileNotFoundError: 'command'. That is true of the worktree-guard test only. pre-commit-no-tag-pinned-action_test.py's 7 failures are AssertionError: 0 == 1, from a different cause: the harness passed an absolute /tmp/pytest-.../ path in argv, #13936 made get_staged_files apply the caller's own anchored pattern (^\.github/(workflows|actions)/) to the argv branch, and an absolute path cannot match that anchor. The hook received an empty file list and took its "nothing staged" exit 0. Worse than the 7 reds: the 3 "must allow" tests in that file were passing vacuously, having exercised nothing at all — mutation E below shows they now go red when the hook stops allowing first-party actions, which they could not do before.

The wiring guard caught its own author, twice. The first version of hook_suites_run_in_ci_test.py derived its invocation set by scanning the workflows — and immediately went red on marker-tests.yml, a fourth invocation neither the review nor I had listed. Then the failure exposed that the sweep was only half-derived: it matched python -m pytest and nothing else, while the CI tree actually contains 27 bare pytest, 19 python -m pytest and 1 python3 -m pytest. It reached marker-tests.yml only because that file happens to use the middle spelling — 44 of 64 invocations were invisible to it. A guard narrower than its own subject reads as coverage. It now derives on three axes: all three spellings, .yml and .yaml, and .github/actions/ composite actions as well as .github/workflows/.

Then wiring marker-tests.yml turned out to be the wrong fix, and CI said so — 20 failed / 19 errors, every one fixture 'backend_url' not found in a backend integration test, nothing to do with hooks. That workflow documents itself as a known-red suite kept off the PR gate until #13286, and carries pull_request: paths: ['.github/workflows/marker-tests.yml'] precisely so editing it runs it. My edit dragged an unrelated red suite into this PR's gate and bought nothing — because it runs -m "integration or slow or distributed or performance", a positive selection, and no hook test carries any of those four markers. Adding the path there collects 18 modules and selects zero tests.

So the invariant was too coarse. It is not "every invocation that names repo_tests" but "every invocation that can select the hook suites", which is readable from each invocation's own -m expression. $MARKER_EXPRESSION is resolved from the env block in its own file, and an expression that cannot be resolved is a hard failure rather than an exemption — unknown must not read the same as "does not need it". The exemption is earned: the guard re-derives whether any hook test carries one of the four markers, and the moment one does, marker-tests.yml stops being exempt. marker-tests.yml is restored byte-identical to base (blob 6281b27ca), so this PR no longer triggers it.

Review finding, and it was right. "The 8 tests now run" was true only by hand. pytest.ini's testpaths listed 8 roots and autobot-infrastructure was in none of them; neither was it in ci.yml, coverage.yml or test-durations.yml; and autobot-infrastructure/shared/tests/pytest.ini sets testpaths = unit integration e2e, which does not reach shared/scripts/. So all 18 suites were collected by nothing, and the fail-closed proof for git worktree list would have been verified exactly once — by hand, by me, in the PR claiming to restore it. That is the same shape this PR closes twice elsewhere, sitting inside the PR. Wired into all four places; mutation J below shows the pre-PR invocation is green on a broken fail-closed path and the wired one is red.

One more defect surfaced while deriving the rule. slm-post-commit is 100755 but carries its #!/bin/bash on line 4, below the copyright header, where it is a comment. execve refuses a file with no shebang and git falls back to sh, so a bash-only hook changed meaning silently. Shebang moved to line 1, and the guard now asserts the rule in both directions.

What Changed

Index modes

Set with git update-index --chmod=+x — a plain chmod does not reach the index under core.fileMode=false.

File	Before	After
tools/git-hooks/pre-push	100644	100755
tools/git-hooks/pre-commit	100644	100755
tools/git-hooks/install_hooks.sh	100644	100755
scripts/install-git-hooks.sh	100644	100755
autobot-infrastructure/shared/scripts/hooks/function_length_checker.py	100644	100755
autobot-infrastructure/shared/scripts/hooks/slm-post-commit	100755	100755 (unchanged — shebang moved to line 1)
tools/git-hooks/README.md	100644	100644 (unchanged — not executable)

Verified with git ls-tree -r origin/Dev_new_gui against git ls-files -s on this branch.

#14909 — the hooks

• repo_tests/git_hooks_executable_test.py — reads the index mode via git ls-files -s and asserts a derived rule in both directions: a file declaring a shebang must be 100755, and a file that is 100755 must declare a shebang. Four reach floors, including an explicit extensionless count.
• repo_tests/git_hooks_installer_test.py — drives the real installer from inside a real linked worktree, deletes the worktree, and checks the hook is still a file, still executable and still byte-identical to its template. A second case starts from the state the repo was actually found in — a dangling symlink at the destination — and checks a re-run replaces it. git-hook enforcement silently dead: dangling pre-push symlink into deleted worktree; no pre-commit installed under explicit core.hooksPath #11598 made the installer copy-based, but nothing had ever asserted the property.
• tools/git-hooks/README.md — the "the hooks are symlinked, so edits are picked up on the next push" section has been wrong since git-hook enforcement silently dead: dangling pre-push symlink into deleted worktree; no pre-commit installed under explicit core.hooksPath #11598; it now says to re-run the installer and documents the update-index spelling.
• .github/filters/python-paths.yml — found while watching this PR's own CI. python-suite is gated by that filter, and everything the three new guards assert on is a shell script or a file mode: no Python at all. A PR flipping pre-push back to 100644, or putting || true back into an extensionless hook, would have matched no pattern — the shim computes python != 'true', publishes the required python-suite context green, and the guard written to catch exactly that never runs. The guards would have been inert on the only diffs they exist for. This is the shape that file's own header already documents for tech-debt(infrastructure): 18 sys.path bootstraps open-code the project root and default to the live install instead of the checkout #14544/fix(infrastructure): 18 sys.path bootstraps default to the live install instead of the checkout (#14544) #14575, so the fix is the same: name the inputs. test_the_python_filter_reaches_the_inputs_these_guards_read evaluates the globs against real paths rather than looking for the strings, because an entry that is present but no longer matches is worth nothing.

#14891 — the lib-source guard

• Shell detection by shebang, not suffix or a name list.
• The deliberately-exit-0 category, asserted on behaviour and bounded above.
• All floors re-derived; a new extensionless floor and a named extensionless call site.
• post-commit-doc-sync:16 — sourced with no failure block at all; it failed closed only because of the incidental set -e at the top of the file. Now || { … exit 1; }.
• pre-commit-warn-untracked:49 — the decisive attempt was if ! source … 2>/dev/null, discarding the library's own stderr, so a developer was told the load failed but never why. Redirect removed; the exit 0 stays and is now declared.

#14884 — the hook suites

• pre-commit-worktree-branch-guard_test.py — subprocess.run(["command", "-v", "git"]) → shutil.which("git"). command is a shell builtin; subprocess given a list never involves a shell. The old call's result was never even read.
• pre-commit-no-tag-pinned-action_test.py — the file path now goes in repo-relative with cwd at its root, the only shape pre-commit or a CI wrapper produces.
• repo_tests/hook_suite_harness_test.py — answers the issue's last acceptance criterion by AST, not grep, so a builtin named in a docstring is not a false positive.
• The suites are now actually collected. pytest.ini testpaths + all three CI invocations (ci.yml, coverage.yml, test-durations.yml) now name autobot-infrastructure/shared/scripts/hooks. Measured before wiring, so this is a wiring fix and not a quarantine: 159 collected, 159 passed under this repo's own pytest.ini (importlib import mode, strict markers). No tech-debt(tests): cross-backend + conftest-stub namespace pollution causes ~92% of full-suite collection errors and ~1400+ runtime failures (prereq for #10691) #13084-style package-name collision — the suites import only stdlib plus pytest, carry no __init__.py, and need no separate interpreter invocation. Sharding was checked too: stable_shard buckets every collected module by path hash, so a module absent from .test_durations is still assigned to a shard rather than silently dropped.
• Narrow on purpose. autobot-infrastructure as a whole does not collect: 23 collection errors, and shared/scripts/test_alertmanager.py calls sys.exit(0) at import, which takes pytest down with an INTERNALERROR rather than a test failure. Filed as bug(ci): 56 of 74 test files under autobot-infrastructure are collected by nothing, and one kills the pytest session #14917 (56 of 74 infra test files collected by nothing, plus a second tools/scripts/pipeline-scripts drift between the same three invocations) rather than bundled here.
• repo_tests/hook_suites_run_in_ci_test.py — guards both halves, because either alone passes while the wiring is broken. Configuration: the invocation set is derived by scanning the CI tree — all three pytest spellings, both YAML extensions, workflows and composite actions — never from a list of filenames. Measured: 64 invocations across 61 files, of which exactly 4 name repo_tests as a bare path token (ci.yml, coverage.yml, marker-tests.yml, test-durations.yml); -p repo_tests.stable_shard is correctly not one of them. Carrier floor sits AT 4; the invocation floor at 40 catches a scanner that has stopped matching rather than letting it report an empty offender list. Behaviour: it runs the real collector under this repo's pytest.ini, because a path listed in a config that then ignores it collects nothing and exits zero.
• Three invocations are wired — ci.yml, coverage.yml, test-durations.yml. marker-tests.yml is the fourth carrier and is correctly not wired: it selects only marked tests, and the hook suites carry none of those markers.

Carrier	-m expression	Selects unmarked?	Hooks path
ci.yml	not integration and not slow and …	yes	wired
coverage.yml	not integration and not slow and …	yes	wired
test-durations.yml	not integration and not slow and …	yes	wired
marker-tests.yml	integration or slow or …	no	exempt, and the exemption is re-derived

• python-paths.yml now says autobot-infrastructure/** rather than two shell-shaped patterns (review's structural note). shell_lib_sources_resolve_test.py finds subjects by shebang, and "has a shebang naming a shell" cannot be written as a glob — a new extensionless script elsewhere under that tree would have matched neither pattern and taken the shim's green. The filter test's subjects are now derived from git ls-files over the roots the guards read (646 non-Python files) instead of three hand-picked paths.

Verification

Every number below was measured, not estimated. Guards and mutations were run against a scratch copy of the tree outside the repository; nothing was mutated in the worktree and no mutation is in any commit.

Measured reach — guarded / total

Guard	Before	After
lib-source: distinct scripts with a lib site	55 / 71	71 / 71
lib-source: source sites	61 / 78	78 / 78
lib-source: shell scripts walked	160	180
lib-source: decisive sites classified	56	73
exec bit: shebanged files at 100755	20 / 25	25 / 25
exec bit: extensionless files covered	0	22
harness: hook suites parsed / spawn sites	—	18 modules, 67 sites, 0 offenders

The +16 distinct scripts are exactly the 16 extensionless hooks #14891 enumerated.

Suites, before and after

pre-commit-no-tag-pinned-action_test.py + pre-commit-worktree-branch-guard_test.py, on a clean copy of base: 8 failed, 10 passed, reproducing the issue exactly. After the harness fix: 18 passed.

The five new/changed repo_tests modules plus the widened guard: 93 passed. With the newly-wired hook suites included: 252 passed.

Local interpreter: CPython 3.10.12, pytest 9.0.2. Counts are interpreter-specific; CI is the authority.

Mutation — every guard was broken on purpose and confirmed red

#	Mutation	Result
M1	git update-index --chmod=-x tools/git-hooks/pre-push	2 failed — test_every_shebanged_hook_is_tracked_executable, test_the_three_hooks_that_were_inert_are_executable
M2	push slm-post-commit's shebang back below the header	2 failed — incl. test_no_executable_hook_relies_on_a_missing_shebang
M3	reintroduce || true in an extensionless hook (pre-commit-branch-guard:30)	2 failed — the #14891 acceptance mutation
M4	restore 2>/dev/null on warn-untracked's decisive attempt	1 failed — test_a_missing_lib_is_never_swallowed
M5	drop the lib-source-contract marker (bare exit 0)	3 failed — the category is not free
M6	narrow the walk back to rglob("*.sh")	5 failed — the widening is load-bearing
M7	reintroduce ["command", "-v", "git"]	1 failed — test_no_hook_suite_spawns_a_shell_builtin
A	hook stops rejecting third-party tag pins	7 failed — the supply-chain property is live again
B	hook stops allowing first-party actions	5 failed — proves the 3 "allow" tests are no longer vacuous
C	worktree guard swallows a git worktree list failure	1 failed — test_a_git_worktree_list_failure_does_not_report_clean
D	installer symlinks instead of copying	2 failed
E	installer becomes a no-op	2 failed — absent does not read as clean
F	drop the hook entries from python-paths.yml	1 failed — the guards would not have gated their own subject
G	keep the entries but misspell one (tools/git-hook/**)	1 failed — the check evaluates the glob, it does not grep for it
H	drop the hooks path from ci.yml's invocation	1 failed — test_every_ci_pytest_invocation_that_runs_repo_tests_also_runs_the_hooks
I	drop the hooks path from pytest.ini testpaths	1 failed — and it exposed a sloppy parser in my own test, which split on the first testpaths inside a comment; now parsed with configparser
J	break the fail-closed path in pre-commit-worktree-branch-guard	pre-PR invocation (repo_tests only): 93 passed, green — the regression ships. Wired invocation (repo_tests + the hooks path, what ci.yml now runs): 1 failed, precisely test_a_git_worktree_list_failure_does_not_report_clean. Reverted: 252 passed.
K	narrow python-paths.yml back to the shell-shaped patterns	1 failed — the derived subject list catches what three hand-picked paths did not
L	un-wire marker-tests.yml	1 failed, naming it exactly — this is the mutation CI performed for real on 53f4d306f
M	narrow the invocation regex back to python -m pytest only	1 failed on only found 19 pytest invocations — the guard cannot silently shrink its own reach
N	un-wire ci.yml under the new marker-aware rule	1 failed, naming ci.yml exactly
O	give a hook test @pytest.mark.integration	1 failed — the exemption dies and names pre-commit-branch-guard_test.py:48; the exemption is earned, not asserted
P	make $MARKER_EXPRESSION unresolvable	1 failed — "unknown is not an exemption", rather than silently exempting
Q	switch marker-tests.yml to the exclusion expression	1 failed — the marker-only branch would be untested, so the classification cannot quietly collapse to one case

Each mutation was reverted and the suite returned to green before the next.

Not verified here

The exec-bit fix is a tracked-mode change; whether a given developer's .git/hooks/ is repaired depends on them re-running bash scripts/install-git-hooks.sh. That is per-clone state this PR cannot reach, and the README now says so.

Residual filed

#14920 — 262 tests across the repo return a value instead of asserting, so they pass whichever branch they take (backend_startup_e2e_test.py::test_imports returns a bool). Surfaced by a PytestReturnNotNoneWarning in this PR's own CI log. Counted by AST, attributing a return only to the test that directly encloses it — a naive walk inflates it to 739. Zero of them are in the directory this PR wires into CI, so this branch neither introduces nor inherits any.

#14917 — 56 of the 74 test files under autobot-infrastructure/ are still collected by nothing, shared/scripts/test_alertmanager.py kills a pytest session with INTERNALERROR at import, and coverage.yml/test-durations.yml omit the tools/scripts/pipeline-scripts roots that ci.yml carries. Not bundled here: it needs 23 collection errors triaged first, which is a different size and risk of change. Cross-linked both ways.

Note the reviewer's "all three pass the same explicit path list" is very slightly off — ci.yml carries three roots the other two do not. The hooks omission was identical in all three and is fixed in all three; the other drift is #14917.

Model Used

Claude Opus 5 (1M context) — claude-opus-5[1m]

[github-actions]

✅ SSOT Configuration Compliance: Passing

🎉 No hardcoded values detected that have SSOT config equivalents!