fix(hooks): a stale lastfailed entry must not run the whole suite (#984) - #1102
Conversation
`pytest --lf --lfnf none` guards only the EMPTY cache. With a non-empty
lastfailed whose node IDs no longer resolve — a test renamed, moved or
deleted since it failed — pytest falls back to running everything.
Reproduced exactly as the issue describes:
Case C (target exists): --lf --lfnf none -> 1 test ran
Case A (target renamed): --lf --lfnf none -> all 3 ran
So the hook silently converted from "run nothing" to "run the entire
suite", carrying -x. Self-triggering in the most ordinary workflow: rename
a test and the very next commit hangs.
`scripts/pretest_lastfailed.py` computes the selection itself — drop node
IDs that can no longer be collected, run what is left. Collection is
scoped to the files named in the cache, so it can never become the
full-suite pass it exists to prevent. Case A now selects nothing and exits
0; Case C still selects exactly the failure and still blocks the commit.
Chose the issue's option 1 over its weakly-preferred option 2: pre-commit
passes CHANGED files, which are mostly source files, and `pytest
codeframe/core/workspace.py` collects nothing. Option 2 therefore degrades
to "run the test files you edited" and drops the hook's actual value —
re-running what you previously broke after a source change, the common
case.
Option 3 taken as well: the hook now runs under `timeout 300`, and 124 is
translated to a pass with a warning. A pre-commit hook needs a wall-clock
ceiling whatever it decides to run.
Fails safe throughout — an unreadable or malformed cache selects nothing.
The one outcome this must never produce is "I could not tell, so run
everything".
…984) codex review [P2]. My own comment said "collection errors are fine" — wrong, and the mistake was conflating two things that look identical from the outside. A cached node ID that no longer resolves because the test was renamed is *stale*: nothing to run, prune it. A cached file that raises during collection because you just made it un-importable is *broken* — and that is precisely what a pre-commit hook exists to catch. Pruning the second along with the first meant a syntax error in a previously-failing test file produced an empty selection and exit 0, letting the commit through. `collectible` now returns the errored files separately, and any cached node ID whose file errored selects the file itself, so pytest re-raises the collection error and blocks the commit. Verified against real pytest output rather than assumed: a collection failure is bannered as "ERROR collecting <path>" with rc=2. Four tests: a syntax error is selected and fails the hook, an import error counts the same, and a broken file does not drag a genuinely-stale neighbour back into scope.
|
Claude finished @frankbria's task in 6m 42s —— View job GLM precision review — bug hunt
Scope: concrete defects only (logic errors, unhandled crash/loss paths, contract breaks). Style/architecture/coverage are covered by the general review and intentionally not repeated here. What I examined closely:
✅ GLM review: no defects found.The one theoretical gap I considered — |
Third-party review —
|
| symptom | actual meaning | right response |
|---|---|---|
| cached node ID no longer resolves, file collects fine | test was renamed — stale | prune, run nothing |
| cached file raises during collection | you just made it un-importable | run it, block the commit |
Pruning the second along with the first meant a syntax error in a previously-failing test file produced an empty selection and exit 0 — the hook passing on precisely the change it exists to stop. Worse than the bug I was fixing, in the sense that a hanging hook is at least visible.
Fixed by making collectible() return the errored files separately, verified against real pytest output rather than assumed (ERROR collecting <path>, rc=2). Four tests: syntax error selected and hook fails, import error the same, and — the one that pins the distinction — a broken file does not drag a genuinely-stale neighbour back into scope. Mutation-checked by forcing errored = set(), which fails all four.
Pass 2 (final diff) — clean:
The changes are narrowly scoped to the pre-commit last-failed pytest hook and add tests covering stale cache, live failures, malformed cache, and collection-error cases. I did not find a regression or actionable bug introduced by this diff.
No Critical or Major findings outstanding.
One thing not caught by a reviewer, worth recording: my first .pre-commit-config.yaml edit produced invalid YAML and I nearly didn't notice, because the hook simply wouldn't have run. The entry: value is an unquoted plain scalar, and I put a : inside it (echo "pytest-check: timed out..."), which YAML reads as a nested mapping. Now written as a >- folded block, and verified by parsing the config and printing the composed entry string rather than eyeballing it — a broken hook config fails open, so "it didn't complain" is not evidence.
ReviewNice, disciplined fix — clear repro, the stale-vs-broken distinction is exactly right, "fails safe = select nothing, never everything" is a good invariant, and the Likely correctness bug:
|
claude-review, and this one silently defeated the whole fix in THIS repo. Verbosity in pytest is a single counter. `pytest.ini` here starts addopts with `-v`, so the probe's `-q` netted to DEFAULT verbosity, and `--collect-only` then prints an indented <Module>/<Function> tree with no "::" on any line. Every recorded node ID parsed as unresolvable, so the hook selected NOTHING — including tests that were genuinely still failing. Measured against the real cache: 298 entries, 0 selected. After the fix, 5 selected and 293 correctly pruned as stale. My tests missed it because the fixture wrote a bare `[pytest]` ini. Every case is now parametrized over that AND an ini that starts addopts with `-v`, reproducing this repo's actual configuration. `-o addopts=` is the fix, and it is load-bearing rather than tidiness: it also drops `-m` filters and coverage flags, none of which belong in a collection probe. Second, more important change: the parser no longer treats "I found no node IDs" as "everything is stale". If pytest reports collecting tests but nothing parses, that is an output shape this script does not understand, and it now says so on stderr and falls back to running the cached files — still bounded to the handful named in the cache, never the suite. A conftest can raise verbosity in code where no flag can undo it, so `-o addopts=` alone is not a complete answer. Silently selecting nothing is the exact failure mode this issue is about. It should not be reachable by a parsing surprise.
This one silently defeated the entire fix, in this repo, and my tests could not see it
Confirmed exactly, against the real cache: After So the hook I shipped to stop a full-suite run had been converted into a permanent no-op — arguably worse than the bug, because a four-hour hook is at least visible. Why my tests missed it: the fixture wrote a bare
The more important changeDiagnosing this made the deeper flaw obvious: the parser treated "I found no node IDs" as "everything is stale". Those are not the same claim, and conflating them is how a parsing surprise becomes a silent no-op — the precise failure mode this issue exists to fix. So the parser now refuses to guess. If pytest reports collecting tests but nothing parses, it says so on stderr and falls back to running the cached files — still bounded to the handful named in the cache, never the suite. This is not hypothetical: Building it also caught a bug in the guard itself — my count regex was 49 tests now. Mutation-checked: dropping |
|
Claude finished @frankbria's task in 6m 33s —— View job GLM precision review — bug hunt (latest commit)
Scope: concrete defects only (logic errors, unhandled crash/loss paths, contract breaks). Style/architecture/coverage are covered by the general review and intentionally not repeated here. Reviewed the final commit ( What I examined closely:
✅ GLM review: no defects found.The one imprecision I considered — the |
ReviewThis PR went through three solid rounds of self-correction (GLM automated pass, codex review, and a prior Claude review), each catching a real bug that the previous round missed — the collection-error-vs-staleness conflation, and then the I was not able to execute the test suite or the script directly in this sandbox (command execution required approval that wasn't available), so the below is a manual trace of the code, not a re-run of the 29 parametrized tests. Correctness — looks solid after the third commit
Minor / non-blocking
Other
Nice, disciplined iteration overall — the repo's own config surfaced a real gap in the first fix, and the response (parametrize the test fixture over the actual environment, not just clear the flag) fixed the root cause rather than papering over the symptom. |
…984) claude-review, narrow but real. The guard was gated on `not resolved and not errored`, but `errored` comes from an "ERROR collecting" regex that is verbosity-independent, so it stays populated while `resolved` goes empty under forced verbosity. A cache mixing a broken file with a healthy-but-still-failing one therefore slipped past the guard, and the healthy file's live failure was pruned as stale. Dropping `not errored` closes it without a new failure mode: if every file errored, pytest reports 0 collected, `_collected_something` is False, and the guard correctly stays quiet. Pinned by test_it_fires_even_when_one_file_also_errored; restoring the old condition fails it.
|
Claude finished @frankbria's task in 4m 27s —— View job GLM precision review — bug hunt (latest commit)
Scope: concrete defects only (logic errors, unhandled crash/loss paths, contract breaks). Style/architecture/coverage are covered by the general review and intentionally not repeated here. Reviewed the new synchronize commit ( The diff is one condition change in - if not resolved and not errored and _collected_something(result.stdout):
+ if not resolved and _collected_something(result.stdout):What I examined closely:
✅ GLM review: no defects found.The fix is narrow and sound: it removes exactly the term that let a verbosity-forced + partially-broken cache slip past the loud-fallback guard, and the term's removal cannot create a new failure mode because the fallback it gates is itself fail-safe (bounded to cached files, blocks on real failures). No inline findings. |
|
All three addressed. Guard gap (mixed broken + live under forced verbosity) — fixed in 23f7350. You're right that I'd have accepted "acceptable residual risk" on the likelihood argument, but the fix was one condition and the test was five lines — cheaper to close than to document. Test-count drift — corrected. The honest number is 23 test functions → 46 collected, since the fixture is parametrized over both ini variants. The "49"/"51" in my commit messages were whole-directory runs that also picked up Known-limitations wording — you're right, and my description had the mechanism backwards. While correcting it I noticed something worth stating explicitly rather than leaving for a future reader to discover: the five node IDs this repo currently selects are e2e tests, which the hook will run even though the CI gate excludes them. That is unchanged from |
ReviewRead the full diff (all 4 commits) independently rather than re-litigating the three rounds already caught by GLM/codex/Claude (collection-error-vs-staleness conflation, the One thing not yet raised: a mixed selection can abort the real run entirely, not just prune wrong
Without This isn't a safety regression — the invariant that matters (never silently pass) still holds, since pytest exits nonzero on the collection error and the commit is still correctly blocked. But it means the developer sees a collection-error abort, not the actual assertion failure they were expecting to see re-run, which could be confusing to debug ("why didn't my failing test even show up?"). Given how much rigor already went into distinguishing stale-vs-broken, might be worth a one-line note in the module docstring/known-limitations, or splitting the real run into two invocations (errored files, then resolved node IDs) if you want the healthy failure's own output to survive a neighbour's breakage. Not blocking — just flagging since none of the prior rounds exercised Everything else
Nice, disciplined iteration overall — each round's self-correction fixed the actual root cause rather than papering over the symptom. |
…to see (#984) claude-review, and the first finding in this thread about main()'s real run rather than select()'s output — the prior rounds all asserted on --print-selection, so none of them exercised this. A mixed selection (a broken whole-file entry plus a live node ID from a different, healthy file) went to one invocation: pytest -x test_broken.py test_gt.py::test_beta_fails Without --continue-on-collection-errors, pytest aborts the entire session on the collection error before running anything, so the live failure never executes or reports. Not a safety regression — the exit is nonzero and the commit is still blocked — but the developer sees "1 error during collection" and debugs the wrong thing. Split into two invocations, node IDs first, so the expected assertion failure is what surfaces; a broken file is still reported after, and either blocks the commit. Exit 5 between groups is the deselect-all no-op and is not treated as failure. Three tests, and both the split AND its ordering are pinned: reverting to one invocation fails 2, and running the files first fails the same 2.
|
Fixed in the latest push rather than documented — you were right that this is the one place the prior rounds never looked. Confirmed: all three earlier rounds asserted on Agreed the safety invariant held — nonzero exit, commit blocked — but "blocked for a reason you can't see" is its own kind of bad in a hook whose entire purpose is telling you what you broke. Given how much of this PR is about silent wrong behaviour, leaving a second "the output lies to you" case in place would have been inconsistent. Took the split-into-two-invocations option, node IDs first, so the expected assertion failure is what surfaces; a broken file is still reported after, and either blocks the commit. Exit 5 between groups is the deselect-all no-op and isn't treated as failure. Both the split and its ordering are pinned, since the ordering is the part that's easy to regress silently: reverting to a single invocation fails 2 tests, and running the whole-file group first fails the same 2. Also added the two boundary cases — a broken file alone still fails, and a broken file is still reported when the live tests all pass (fail-fast must not hide it behind a green run). 57 tests in the directory now. That makes four rounds where the review found something real; the reviewers on this one earned their keep. |
|
Claude finished @frankbria's task in 4m 21s —— View job GLM precision review — bug hunt (latest commit)
Scope: concrete defects only (logic errors, unhandled crash/loss paths, contract breaks). Style/architecture/coverage are covered by the general review and intentionally not repeated here. Reviewed the new synchronize commit ( The diff is the body of What I examined closely:
✅ GLM review: no defects found.The one theoretical path I considered — a cache entry beginning with |
ReviewThis has already been through five rounds of adversarial review (GLM x3, codex, Claude x4) that caught real, non-overlapping bugs at each step -- the collection-error-vs-staleness conflation, the One residual edge case in
|
…984) claude-review is right that the behaviour was unverified either way and my own wording oversold it. I said "a broken file is still reported after", but -x applies across the groups: if a live test fails we return immediately and never reach the broken neighbour. That is the fail-fast contract, not an oversight — the hook exists to show you the first thing you broke, and the broken file surfaces on the next attempt once that is fixed. Running both groups regardless would contradict the -x the hook has always carried. So: behaviour unchanged, claim corrected, and the early return now pinned by a test asserting the live failure IS reported and the collection error is NOT — so a future reader sees it as deliberate rather than a missing loop.
|
Good catch on the imprecision — my wording, not the code. You're right that "a broken file is still reported after" isn't true when the live test fails: I'm keeping the behaviour and pinning it, rather than making both groups run. The hook has always carried What was genuinely missing is that neither branch was verified — as you note, 59 tests. Five rounds, five real findings — worth saying that none of them overlapped, which is unusual. |
|
Claude finished @frankbria's task in 5m 35s —— View job GLM precision review — bug hunt (latest commit)
Scope: concrete defects only (logic errors, unhandled crash/loss paths, contract breaks). Style/architecture/coverage are covered by the general review and intentionally not repeated here. Reviewed the final commit ( What I examined closely:
✅ GLM review: no defects found.The one code-level imprecision I considered — the |
ReviewI read the full diff plus the review history — six rounds already caught and fixed real, non-overlapping bugs: the collection-error-vs-staleness conflation, the Independent pass over the current (final) state, focused on things prior rounds may not have exercised:
I didn't find anything new to add. Test coverage is genuinely strong for a script this size (23 functions × 2 ini variants = 46 cases, plus the mixed/format-fallback/ordering edge cases that came out of the review rounds), and the fail-safe invariant ("unreadable/ambiguous cache → select nothing, never everything") is consistently upheld everywhere I traced it. Good to merge from a correctness standpoint. |
1 participant
Closes #984.
Reproduced first
--lfnf noneguards only the empty cache. With a non-emptylastfailedwhose node IDs no longer resolve, pytest falls back to running everything — carrying-x, so it also aborts the commit on the first unrelated failure it hits. Self-triggering in the most ordinary workflow: rename a test, and the very next commit hangs.Worth noting from the repro: the cache grows a second key rather than replacing one. After the rename it held both
test_beta_failsandtest_beta_renamed, so the stale entry never clears itself.Fix
scripts/pretest_lastfailed.pycomputes the selection instead of trusting--lf: readlastfailed, drop node IDs that can no longer be collected, run what's left. Collection is scoped to the handful of files named in the cache, so it can never become the full-suite pass it exists to prevent.Before vs after, same repro, clean caches:
--lf --lfnf none)1 failed, 2 passed— all 3 ran1 failedtest_gt.py::test_beta_fails, exit 1Case C still blocks the commit, which is the property that must not regress.
Why option 1, not the issue's weakly-preferred option 2
Option 2 ("scope to changed files") changes what the hook means. Pre-commit passes changed files, which are mostly
codeframe/**source files —pytest codeframe/core/workspace.pycollects nothing. So option 2 quietly degrades to "run the test files you edited" and drops the hook's actual value: re-running what you previously broke after a source change, which is the common case.Option 3 taken as well, as the issue suggests: the hook runs under
timeout 300, and exit 124 becomes a pass with a warning. A pre-commit hook needs a wall-clock ceiling whatever it decides to run.Fails safe
An unreadable or malformed cache selects nothing. The one outcome this must never produce is "I couldn't tell, so run everything" — pinned by
test_malformed_cache_selects_nothing_rather_than_everything.The review caught a real hole I opened
codex [P2]: my first version pruned any node ID that didn't collect — and my code comment said "collection errors are fine". Wrong, and the mistake was conflating two things that look identical from outside:
Pruning the second along with the first meant a syntax error in a previously-failing test file gave an empty selection and exit 0 — the hook passing on the very change it should stop.
collectible()now returns errored files separately (verified against real pytest output:ERROR collecting <path>, rc=2), and a cached node ID whose file errored selects the file so pytest re-raises and blocks the commit.Verification
tests/scripts/test_pretest_lastfailed_984.py(every case parametrized over a plainpytest.iniand one whoseaddoptsstarts with-v, matching this repo) — Case A (rename / deleted file / deleted test), Case C (exact selection, no neighbours, mixed stale+live), empty/missing/malformed cache, whole-file keys, the four broken-file cases, the five unparseable-format cases, and end-to-end runs asserting exit codes.-o addopts=fails 3; making the format guard prune silently fails 6; gating that guard onnot erroredfails 2.pre-commit run pytest-check --all-files→Run last failed tests (fast feedback)....Passed. YAML validated by parsing the config and printing the composedentry(the first attempt broke it — a:inside an unquoted plain scalar).ruff checkclean.Known limitations
.git/hooks/pre-commitis a secret scanner, not the pre-commit framework's hook, so.pre-commit-config.yamlonly takes effect where someone has runpre-commit install. That's pre-existing and out of scope; the config is the tracked artifact and is now correct.collectible()clearsaddopts, so it still resolves at selection time. It is neutralized later: the real run inmain()keepsaddopts, pytest exits 5 ("no tests ran"), and the YAML wrapper maps that to a pass. Same outcome, different mechanism than an earlier draft of this note claimed.--lf's behaviour and bounded by the 300s ceiling, but it is a real difference between the hook and the CI gate.