[P2.19] pre-commit `pytest --lf` runs the FULL suite when a lastfailed test was renamed — `--lfnf none` doesn't cover it

[frankbria]

Surfaced while implementing #896 (PR #980), where it blocked a commit for ~15 minutes before I killed it.

Problem

The pytest-check pre-commit hook is designed as a fast-feedback no-op when nothing previously failed:

# .pre-commit-config.yaml
entry: bash -c 'uv run pytest --lf --lfnf none -x; code=$?; [ "$code" -eq 5 ] && code=0; exit $code'

--lfnf none guards exactly one case: the lastfailed cache is empty. It does not guard the case where the cache is non-empty but its node IDs no longer resolve — a test that was renamed, moved, or deleted since the last failing run. In that case pytest falls back to its documented behaviour of running everything.

So the hook silently converts from "run nothing" to "run the entire suite" — and with #979 (full local suite ≈ 4h vs 4m38s on CI) that is a multi-hour pre-commit hook. It also carries -x, so it aborts the commit on the first unrelated failure it happens to hit.

This is self-triggering in the most common workflow: you rename a test, the old name is still in lastfailed from the run where it failed, and your very next commit hangs.

Reproduction

Minimal, self-contained — no repo state needed:

mkdir -p /tmp/gt && cat > /tmp/gt/test_gt.py <<'EOF'
def test_alpha_passes(): assert True
def test_beta_fails(): assert False
def test_gamma_passes(): assert True
EOF

# Produce a genuine lastfailed cache
uv run pytest /tmp/gt -o cache_dir=/tmp/gtcache -q
# -> 1 failed, 2 passed;  lastfailed == {"test_gt.py::test_beta_fails": true}

# Case C — target still exists: correctly selects ONLY the failure
uv run pytest /tmp/gt --lf --lfnf none -o cache_dir=/tmp/gtcache -q
# -> 1 failed                      ← 1 test ran

# Case A — rename the target away, cache now stale
sed -i 's/def test_beta_fails/def test_beta_renamed/' /tmp/gt/test_gt.py
uv run pytest /tmp/gt --lf --lfnf none -o cache_dir=/tmp/gtcache -q
# -> 1 failed, 2 passed            ← ALL 3 tests ran

Case A is the bug: --lfnf none is in effect and the hook still ran the whole directory.

Confirmed at repo scale too. With a stale entry in the cache:

$ uv run pytest tests/cli/ --lf --lfnf none --collect-only -q -o cache_dir=<stale>
483 tests collected            ← whole directory selected

$ uv run pytest tests/cli/ --lf --lfnf none --collect-only -q -o cache_dir=<empty>
run-last-failure: no previously failed tests, deselecting all items.
no tests collected (483 deselected)     ← the intended no-op

Real-world occurrence

In PR #980 I renamed test_server_module_does_not_read_workspace_root →
test_only_the_allowlist_parser_reads_workspace_root. The old name stayed in
.pytest_cache/v/cache/lastfailed, and the next git commit hung on
Run last failed tests (fast feedback) for 15+ minutes (killed, not completed).
Clearing the stale entry made the same commit's hooks pass in seconds.

Suggested fix

Options, roughly in increasing order of effort:

1. Prune unresolvable node IDs before running — cheapest faithful fix. Keeps --lf semantics; only drops entries that can no longer be collected.
2. Scope the hook to changed files (pass_filenames: true) instead of --lf. Arguably what "fast feedback on what you just touched" should mean anyway, and immune to cache staleness entirely.
3. Bound it — wrap in timeout <N> and treat a timeout as a pass with a warning. Doesn't fix the cause, but caps the damage and is one line.

My weak preference is (2), with (3) as a cheap belt-and-braces regardless: a pre-commit hook should have a hard ceiling on wall-clock time no matter what it decides to run. (1) preserves current intent most exactly if that matters more.

Whatever the fix, a regression test should assert the Case A behaviour above — that a stale lastfailed entry does not expand the hook's scope.

Notes

• Not a blocker for anything; workaround is rm .pytest_cache/v/cache/lastfailed, or run the renamed test once so the entry clears itself.
• Interacts with [P2.18] Full test suite takes ~4h locally vs 4m38s on CI — per-test SQLite fsync #979: the same fallback on CI would cost ~5 minutes, locally ~4 hours. Fixing [P2.18] Full test suite takes ~4h locally vs 4m38s on CI — per-test SQLite fsync #979 would reduce the severity but not the defect.
• pytest 8.4.2.

[frankbria]

Hit this three times in one session while working #898 — confirming the diagnosis and adding the exact trigger.

--lfnf none only covers no cache / empty cache. When the cache holds a node id that no longer resolves — a renamed test, a test moved to a different class, or an entry left behind by a run killed mid-test — --lf selects nothing but still collects the entire suite. On this machine that is ~70 minutes per commit (#979), so the hook silently turns every commit into a full-suite run.

Observed entries that triggered it:

• test_non_ascii_configured_token_matches_itself (renamed upstream; no longer exists)
• TestOwnerScopeClamp::test_missing_owner_row_fails_closed + ::test_unreadable_owner_fails_closed (moved to another class in the same commit)

Workaround each time was rm -f .pytest_cache/v/cache/lastfailed.

Suggested fix: have the hook prune unresolvable node ids from lastfailed before invoking pytest, or add --co -q guard, or simply drop the cache when --lf selection count equals the full collection count.