fix(pre-commit): stop full-suite runaway + python-not-found; refine skip detector

[frankbria]

Problem

Two pre-commit hooks were effectively broken (surfaced while committing the #741 fix):

1. pytest-check ran pytest --lf, which runs the entire suite when there's no last-failed cache (a fresh checkout). That's a ~2h18m run — commits were impractical, so people bypassed hooks.
2. skip-detector shelled out to python, which isn't on PATH in this environment → it errored on every run and was silently unenforced.

Fix

• pytest-check: add --lfnf none (re-run only previously-failed tests; no-op when there are none — matching its "fast feedback" name), invoke via uv run (drop the fragile venv-activate/bare-pytest dance), and map pytest's exit 5 ("no tests collected" — the deselect-all no-op) to success so the no-op doesn't fail the commit. 3s no-op instead of a 2h18m suite; full validation stays CI's job.
• skip-detector: python → python3. Once it actually ran, it flagged 3 legitimate platform skipif guards (Windows POSIX perms, node availability). Refined detect-skip-abuse.py to flag only unconditional skips (@skip / @pytest.mark.skip) and constant-true skipif(True) — its actual purpose (stop hiding failures) — while allowing conditional skipif portability gating.

Verification

• pytest --lf --lfnf none -x on a clean cache → exit 5 → wrapper → exit 0 (3s no-op).
• With a real last-failure in cache, the wrapper still exits 1 (commit blocked, as intended).
• Detector: clean on tests/; flags @pytest.mark.skip and skipif(True); allows skipif(sys.platform==...) and string conditions.
• This PR's own commit passed all three hooks (ruff / pytest-check / skip-detector).

Known limitation (separate, pre-existing)

tests/core/test_tools.py::TestRunTests::test_pytest_success_summary fails locally only (green in CI): run_tests prefers uv run pytest, which can't build a bare tmp-dir project. Not a hook issue — filed separately if desired.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 14 minutes

Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ccf93237-c18a-4f9e-bf01-461e51345f58

📥 Commits

Reviewing files that changed from the base of the PR and between 304f433 and a619ffa.

📒 Files selected for processing (3)

• .pre-commit-config.yaml
• codeframe/core/tools.py
• scripts/detect-skip-abuse.py

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/pre-commit-hook-runaway-and-python

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

Reviewed the diff (no prior substantive reviews on this PR to reconcile with — CodeRabbit's comment is just a rate-limit notice with no content).

Summary

Both fixes address real, well-documented problems (2h18m runaway full-suite hook, python missing from PATH breaking skip-detector silently), and the PR body's own verification steps line up with what the diff does. One finding below is worth addressing before merge; the rest are minor/non-blocking.

Findings

1. skipif bypass via string condition (scripts/detect-skip-abuse.py:112-124) — worth fixing
_is_constant_true_condition only flags constant-true when the condition is a non-string ast.Constant (e.g. skipif(True)). But pytest's legacy string-condition form is eval()'d at runtime, so @pytest.mark.skipif("True", reason="...") unconditionally skips the test while sailing through this detector as "legitimate portability gating" — exactly the abuse this tool exists to catch per its own docstring ("prevent AI agents from circumventing failing tests by adding skip decorators"). Given the threat model is an agent gaming the check, a literal string "True"/"1" is a trivial one-token evasion. Consider special-casing string constants that ast.literal_eval/parse to a constant-truthy Python literal (vs. genuine expressions like "sys.platform == 'win32'").

2. No test coverage for the refactored detector — the repo has no test file for scripts/detect-skip-abuse.py (checked — none exists), so this behavior change (bare skip vs. conditional skipif vs. constant-true skipif) ships unpinned. Given this script is a guardrail against AI-agent gaming, a small pytest module asserting each case (including the string-condition gap above) would make future regressions here loud instead of silent.

3. pytest-check now no-ops on fresh/never-run tests (.pre-commit-config.yaml:14-26) — not a bug, but flagging the trade-off explicitly since it's a policy shift, not just a mechanical fix: --lf --lfnf none means the hook only re-runs tests that previously failed locally. A brand-new test that's broken, or any test never run before on this machine, produces exit 5 → no-op → mapped to success. That matches the PR's stated intent ("full validation is CI's job") and is clearly better than a 2h18m hook nobody ran, but it does mean the local hook now provides near-zero coverage on a clean cache. Worth an explicit nod from the team that this is the desired bar for the local hook.

4. Minor consistency nit — pytest-check was fixed to go through uv run (this repo's canonical invocation per CLAUDE.md), but skip-detector was fixed to python3 scripts/detect-skip-abuse.py directly rather than uv run python scripts/detect-skip-abuse.py. Works today since the script is stdlib-only, but for consistency/future-proofing (python version drift, if it ever gains a dependency) uv run would match the pattern just established a few lines above.

5. Minor edge case — _is_constant_true_condition reads the condition from call.args[0] only; skipif(condition=True, reason=...) passed by keyword would have empty call.args and silently pass as "conditional." Unlikely in practice since skipif's condition is conventionally positional, so low priority.

6. Not introduced by this PR, flagging for awareness only — coverage-check (untouched, stages: [manual]) still has the same fragile venv/bin/activate/.venv/bin/activate/bare-pytest fallback chain that just caused the pytest-check breakage. Lower urgency since it's manual-only, but likely has latent similar fragility whenever it's actually invoked.

Nothing here blocks the stated purpose of the PR (unblocking commits), but #1 is a genuine gap in the security property the tool claims to enforce and I'd suggest closing it before relying on the refined detector going forward.

[claude]

Follow-up to my earlier review (this comment covers only what changed since then — commit a619ffa landed after my prior review at 00:00 UTC, so it wasn't in scope).

New commit: run_tests falls back to bare pytest for non-uv projects (codeframe/core/tools.py)

This directly fixes the "Known limitation" called out in the PR body (test_pytest_success_summary failing locally-only), and the root cause is correctly diagnosed: uv run errors on a pyproject.toml with no [project] table, so any workspace that isn't itself a uv-managed project (including the test's bare tmp dir) was silently forced onto a failing path.

Looks correct:

• _is_uv_project checks uv.lock presence first, then parses pyproject.toml for a [project] table, catching OSError/tomllib.TOMLDecodeError → False. That's the right precedence and a safe failure mode (malformed toml → fall back to plain pytest rather than crash the tool call).
• tomllib is stdlib-only and fine given requires-python = ">=3.11" in pyproject.toml — no new dependency.
• Stays consistent with the "core must be headless" rule — no FastAPI/UI imports added.
• The existing test_pytest_success_summary (tmp dir with only [build-system], no [project] table) now exercises the fallback path and is the regression test for this fix.

Minor, non-blocking:

1. No dedicated test exercises the positive path — a workspace with a real [project] table or uv.lock present, confirming uv run pytest is still chosen when it should be. Right now that path is only implicitly covered by "it worked before this diff." A couple of small parametrized cases (uv.lock present / [project] table present / malformed toml) would pin the guard's behavior directly, in the same spirit as the test-coverage gap I flagged for detect-skip-abuse.py in my earlier review.
2. Edge case: if uv.lock exists but pyproject.toml genuinely lacks a [project] table (some workspace/monorepo layouts), _is_uv_project still returns True from the lock-file check alone, and uv run pytest would be attempted and could hit the same "No project table found" error this PR is fixing. Likely rare in practice, so low priority — just flagging since it's the one path not covered by the new logic's own docstring reasoning.

Nothing here blocks merge — this is a solid, well-targeted fix for a real, already-diagnosed bug.