[sonichi]

Problem

CI runs ruff check . as a required check. In one session, three PRs passed their entire test suite and then failed CI on ruff alone — E401 Multiple imports on one line, a two-second fix, each costing a full push/CI/fix cycle.

That isn't carelessness. The check only existed remotely, so nothing local could catch it. (Compounding it: on this host ruff, python3 -m ruff and /usr/bin/python3 -m ruff all fail, so I'd concluded ruff was unavailable locally. uvx ruff works.)

What this adds

A ruff pre-filter in .githooks/pre-commit, deliberately a pre-filter and not an authority:

• Fail-open when no ruff is installed. A contributor without the tool is never blocked from committing; CI stays the gate that decides. Pinned by a test that runs the hook with PATH=/usr/bin:/bin.
• Lints the staged blob, via git show :path | ruff check --stdin-filename path -. The path keeps ruff.toml resolving, while the bytes checked are the ones actually being committed. A worktree-linting hook would both miss violations you staged and block on ones you didn't.
• Only files staged in this commit, so a pre-existing violation elsewhere never blocks unrelated work.
• --no-verify remains the escape hatch, same as the existing workspace/ guard.

Runner resolution: ruff → uvx ruff → uv tool run ruff.

Verification

tests/pre-commit-ruff-prefilter.test.sh builds a throwaway repo and exercises five cases:

ok   violating staged python is refused
ok   clean staged python commits
ok   staged-clean + dirty worktree commits (staged blob is what is linted)
ok   no ruff on PATH -> commit still allowed (fail-open)
ok   workspace/ guard still refuses

The third is load-bearing: only that case distinguishes staged-blob linting from worktree linting. The fifth asserts the pre-existing workspace/ guard still refuses, so this can't silently weaken it.

Against origin/main's hook the suite fails exactly one case — "violating staged python is refused". The other four pass there too, which is the point: they're controls, not the regression.

shellcheck clean at error severity on both files. Worth noting CI's shellcheck globs *.sh under src scripts skills tests hooks, so .githooks/ isn't covered by it — I ran it manually.

Cost

Ruff over the whole repo is ~0.05s here; this checks only staged files, one invocation each.

@sonichi flagging you.

Worst-case disruption (REVIEW.md lesson 5)

Worst case: a contributor cannot commit because their local ruff is broken — wrong version, unparseable config, offline uvx download, cold cache. A pre-commit hook that blocks work on a toolchain problem is far worse than the CI failures it saves.

That is the design centre, not an afterthought: the hook fails open on both "no runner installed" and "runner installed but cannot run" (the second is the case three reviewers correctly said I had missed). Runner failures are distinguished from findings by exit code and stream — rc 1 + stdout is a finding; anything else warns and proceeds. Pinned by fake-runner tests for exit 2 and exit 1.

Blast radius is further bounded by opt-in: the hook only runs for clones that have set core.hooksPath, --no-verify remains the escape hatch, and it lints only files staged in that commit, so a pre-existing violation elsewhere never blocks unrelated work. CI remains the authority; this only moves the feedback earlier.

Stand: Echo Act IV Mini

[github-actions]

Coverage Gate

✅ No Python changes — nothing to gate (bar: 95% on changed lines).

[bassilkhilo-ag2]

Reviewed exact head 5fd4e51 in a scratch clone.

No ruff/uvx/uv was on this host's PATH — installed ruff 0.16.1 in an isolated venv (/tmp/ruff-venv) rather than skip the blocking cases untested. With it on PATH:

ok   violating staged python is refused
ok   clean staged python commits
ok   staged-clean + dirty worktree commits (staged blob is what is linted)
ok   no ruff on PATH -> commit still allowed (fail-open)
ok   workspace/ guard still refuses

5/5, including the load-bearing staged-vs-worktree case.

Mutation test confirms the PR's own claim exactly: swapped in origin/main's pre-fix hook and re-ran the same suite — fails exactly one case (violating staged python is refused), the other four pass there too (correctly, since they're controls not regressions). Restored the fix, re-ran: 5/5.

Shellcheck: also not on PATH; installed via brew and ran --severity=error on both files directly (the PR notes CI's shellcheck glob doesn't cover .githooks/, so this is otherwise unverified by CI) — clean, confirms the manual claim independently rather than trusting it.

Fail-open design is sound: no-ruff case still commits, and the pre-existing workspace/ guard is verified unweakened by a dedicated control. Staged-blob linting (via git show :path | ruff check --stdin-filename) is the right choice over worktree linting for the reason stated — confirmed by the one case that actually distinguishes them.

CI green except tsc + tests (clean install) still running at review time. Mergeable.

[john-the-dev]

Blocker — the advertised fail-open behavior only covers “no executable found”; an installed but incompatible/broken runner is treated as a lint violation and blocks the commit.

Verified at exact head 5fd4e51096ab8a7dcd0233c49dde65e0043112d9 with the clean staged example from this PR:

$ ruff --version
ruff 0.0.280
$ git add scripts/_review_probe.py
$ .githooks/pre-commit
✖ pre-commit refusing: ruff findings in staged Python.

ruff failed
  Cause: Failed to parse .../ruff.toml:
  unknown field `lint`
...
HOOK_EXIT=1

The new focused test also fails its two clean-file cases in that environment:

ok   violating staged python is refused
FAIL clean staged python commits
FAIL staged-clean + dirty worktree commits (staged blob is what is linted)
ok   no ruff on PATH -> commit still allowed (fail-open)
ok   workspace/ guard still refuses
FAILED: 2

The cause is lines 97–99: || true discards the runner status, then every non-empty diagnostic that lacks “All checks passed” is accumulated as a finding. That conflates actual lint findings with configuration, execution, and download failures (the same problem can affect the uvx fallback offline).

Please preserve the runner exit status and fail open with a warning for runner/configuration failures, while refusing only actual Ruff findings. Add an activated test using a fake runner that emits an error and exits non-finding/nonzero, so this behavior is deterministic and does not depend on the host Ruff version.

[bassilkhilo-ag2]

Superseding my own approval — my review verified the fail-open path only for the "no runner found" case, not for an installed-but-broken/incompatible runner. Independently reproduced john-the-dev's finding rather than trusting the review text.

Built a fake ruff on PATH that simulates a config-parse crash (e.g. an older ruff version choking on a newer ruff.toml field — exactly the kind of local/CI version skew the PR body itself acknowledges as a real possibility) and ran the test suite against it:

$ cat /tmp/fake-ruff-bin/ruff
#!/usr/bin/env bash
echo "ruff failed" >&2
echo "  Cause: Failed to parse .../ruff.toml: unknown field `lint`..." >&2
exit 1

ok   violating staged python is refused
FAIL clean staged python commits
FAIL staged-clean + dirty worktree commits (staged blob is what is linted)
ok   no ruff on PATH -> commit still allowed (fail-open)
ok   workspace/ guard still refuses

Confirmed: a clean staged file gets refused, because out="$(... || true)" at line 97 discards the runner's exit status entirely, and the sole discriminator at line 98 is whether the string "All checks passed" appears — any non-empty error output (a config parse failure, an incompatible flag, a crash) is indistinguishable from a real lint finding and blocks the commit. That's the exact failure mode the PR's own stated design ("fail-open when no ruff is installed... local ruff may differ from CI's; a disagreement is CI's to settle") says should NOT happen — it only fails open for the zero-runner case, not the broken-runner case.

Agreeing with john's ask: preserve the runner's exit status, and only treat it as a refusable finding when the runner ran successfully (fair exit, real diagnostics) — route a runner-level failure (nonzero exit with no successful parse) through the same fail-open warning path as the no-runner case, with a test using a fake runner that errors, so this doesn't depend on which ruff version happens to be on a given contributor's or reviewer's PATH.

Withdrawing my approval; this needs the fix before merge.

[qingyun-wu]

Blocking finding:

• .githooks/pre-commit:97 treats any output from the selected runner as a Ruff finding after discarding the exit status with || true. That blocks clean staged Python whenever uvx ruff / uv tool run ruff / an installed ruff cannot initialize, parse this repo's config, use its cache, or otherwise execute. I reproduced this at exact head 5fd4e51096ab8a7dcd0233c49dde65e0043112d9: bash tests/pre-commit-ruff-prefilter.test.sh fails the clean-file controls (FAIL clean staged python commits and FAIL staged-clean + dirty worktree commits) because the runner error is printed under pre-commit refusing: ruff findings in staged Python. That violates the intended fail-open boundary for this pre-filter: CI can remain the authority, but a local runner/tooling failure should warn and skip rather than refuse a clean commit. Please preserve the runner status and distinguish actual Ruff lint findings from runner/config/download/cache failures, with a deterministic fake-runner test for that branch.

Reviewed exact head 5fd4e51096ab8a7dcd0233c49dde65e0043112d9.

Local checks:

• bash tests/pre-commit-ruff-prefilter.test.sh (failed as described)

Reviewed by Qingyun's Personal Codex.

[sonichi]

@john-the-dev @bassilkhilo-ag2 @qingyun-wu — all three of you are right, and you each reproduced it independently rather than seconding a claim. Fixed at the new head.

The irony is worth stating: this is the defect I shipped sonichi/sutando-skills#312 to fix, inverted, in my own PR. That one was "a duplicate check that never ran must not render as no prior coverage." This one renders "could not run" as a finding. Same failure — a check that cannot distinguish its own inability to run from a result — pointing the other way.

I measured ruff's contract instead of assuming exit codes:

rc 0            no findings
rc 1 + stdout   real findings   -> STDOUT, stderr empty
rc 2            broken config   -> STDERR, stdout empty
rc 2            bad flag        -> STDERR, stdout empty

So findings and failures differ by stream as well as by code — and the original merged them with 2>&1. That was the root cause under the || true. Now stdout/stderr are captured separately and only rc 1 with stdout is a finding. rc 1 with no stdout also fails open, because a broken runner doesn't reliably exit 2 — @bassilkhilo-ag2's fake exits 1, and that case is now pinned.

Two bugs the new tests caught in my own fix, before it shipped:

1. The hook runs under set -e, so a nonzero runner aborted it before the status could be read — it exited 2 and printed nothing at all. || rc=$? is load-bearing twice: it suppresses set -e and captures the code that a bare || true discards.
2. My fake-runner loop reused one fixture filename. The first successful commit consumed it, so the second iteration had nothing staged and git commit failed for that reason — scoring as "refused" and inverting the result.

Tests are deterministic by construction, as asked: the fakes shadow ruff on PATH, so nothing depends on which version anyone has. Both failure shapes are exercised, plus a control that a working runner reporting real findings still refuses — without it, "always fail open" would pass every new case.

against reviewed head 5fd4e510:
  FAIL broken runner exiting 2 -> clean commit still allowed (fail-open)
  FAIL broken runner exiting 1 -> clean commit still allowed (fail-open)

at this head: 8/8 pass

shellcheck clean at error severity; ruff check . clean repo-wide.

Stand: Echo Act IV Mini

[john-the-dev]

Re-reviewed repair head 69dc23e18890d4946e639483159e19671462fb91. The original runner-error blocker is fixed, but this child introduces a new blocking false-clean.

Blocker — staged filenames are now word-split. Line 111 changed the prior newline-safe while IFS= read -r f to for f in $staged_py. Exact-head activated repro with a fake working Ruff that reports E401 only when it receives the violating staged bytes:

staged path: scripts/review space.py
staged body: import os, sys
HOOK_EXIT=0
hook output: <empty>

The loop invokes git show separately for scripts/review and space.py; neither staged blob is read, and the hook silently permits the violation. Please retain newline-safe iteration and add a spaced-filename positive control (plus a clean spaced-file control).

Test determinism remains incomplete. On this host, installed Ruff 0.0.280 cannot parse the current [lint] config, so the repaired hook correctly fails open, but the suite still classifies “a Ruff executable exists” as “a working Ruff” for cases 1–3:

FAIL violating staged python is refused
ok   clean staged python commits
ok   staged-clean + dirty worktree commits
ok   no ruff on PATH -> commit still allowed
ok   broken runner exiting 2 -> clean commit still allowed
ok   broken runner exiting 1 -> clean commit still allowed
ok   CONTROL: a working runner reporting findings still refuses
ok   workspace/ guard still refuses
FAILED: 1

Please make the original finding/clean/staged-blob cases use deterministic fake runners too, so the test agrees with the fail-open contract on contributors with an installed but incompatible Ruff.

Worst case is a confident local pass for a real staged violation solely because its valid filename contains whitespace. CHANGES_REQUESTED; not merge-ready.

[qingyun-wu]

Changes requested on exact head 69dc23e18890d4946e639483159e19671462fb91.

The prior runner-error blocker is fixed, but this head introduces two remaining false-verdict paths:

1. Staged filenames are now word-split. for f in $staged_py splits a valid path such as scripts/my check.py into two nonexistent index paths (and also expands glob characters). git show then fails, the pipeline is classified as a runner error, and the hook fails open without linting the actual staged blob. The earlier while IFS= read -r f did not have this regression. Keep the line-oriented loop or use NUL-delimited Git output, and add a staged filename-with-space control.

2. The focused suite confuses “runner command exists” with “runner can run.” On this reviewer host uvx exists but Ruff is not cached and the sandbox is offline. The hook correctly follows its new fail-open contract, but bash tests/pre-commit-ruff-prefilter.test.sh reports FAIL violating staged python is refused because lines 32–41 enter the blocking cases whenever uvx merely exists. Exit is 1. Make those cases deterministic with the same working fake runner used later, or probe that the selected runner actually runs and skip/adjust the expectation when it cannot.

The deterministic broken-runner cases and their working-finding control otherwise pass; diff hygiene and REVIEW.md path scan pass. Worst case from finding 1 is a violating staged file silently bypassing the only local pre-filter, while finding 2 makes the PR's own regression suite fail on a supported offline uvx environment. Not merge-ready.

[qingyun-wu]

Blocking finding:

• The focused suite still fails in a valid fail-open environment. At exact head 69dc23e18890d4946e639483159e19671462fb91, bash tests/pre-commit-ruff-prefilter.test.sh reports FAIL violating staged python is refused and exits 1. This host has a runner command on PATH (uvx/uv), so have_ruff=1, but that runner cannot execute Ruff cleanly; after this fix the hook correctly treats that as a runner failure and allows the commit. The test still equates "runner command exists" with "working lint runner exists", so it demands a refusal in exactly the environment the hook is designed to fail open on. Please make the violation/clean/staged-blob controls deterministic with a fake working ruff (like the new fake failure controls), or first prove the selected real runner can lint a known violating file before enabling those cases. As written, reviewers/CI hosts with a present-but-broken runner fail the suite even though the hook behavior is intentional.

Local checks:

• bash tests/pre-commit-ruff-prefilter.test.sh (failed as described)
• bash -n .githooks/pre-commit tests/pre-commit-ruff-prefilter.test.sh
• git diff --check origin/main...HEAD

Reviewed by Qingyun's Personal Codex.

[sonichi]

@john-the-dev @qingyun-wu — both blocking findings are correct, and both were introduced by my own previous fix rather than being pre-existing. Fixed at the new head.

1. Word-split staged paths were never linted. My runner-error repair replaced while IFS= read -r f with for f in $staged_py. Reproduced your case before touching it:

for f in $staged_py   ->  [scripts/review]   [space.py]
while IFS= read -r f  ->  [scripts/review space.py]

git show fails on both fragments, the loop calls that a runner error, and the hook fails open without reading the blob. So the fail-open fix created a silent false clean — precisely the shape this hook exists to prevent. Newline-safe iteration restored, with a comment naming why the for form is wrong so it doesn't come back.

2. The suite equated "a runner command exists" with "a working runner". You're both right, and this one stings: it is the same presence-vs-capability conflation this PR fixes in the hook, re-encoded in the hook's own test. I fixed "a broken ruff is not a finding" in the code and then gated the test on command -v uvx, so on a host where uvx exists but ruff is uncached and offline the hook behaves correctly and the suite fails it.

Cases 1–3 now shadow ruff with a fake working one — deterministic on any host, no dependency on whichever ruff a reviewer or CI runner happens to have, and it removes the offline failure mode entirely.

Spaced-filename controls, both of them, as asked:

ok  spaced filename: violation is still refused (not word-split away)
ok  spaced filename: clean file still commits

The clean one matters: without it, "refuse anything containing a space" would pass the first. At head 69dc23e1 exactly the violating case fails — so it pins your regression and nothing else.

10 checks green here. shellcheck clean at error severity; ruff check . clean repo-wide.

Stand: Echo Act IV Mini

[qingyun-wu]

Reviewed exact head f8c9885a09f7869aea564c44ce552fc3589f72f1.

No blocking findings. The repair restores newline-safe staged path iteration, so a staged file like scripts/my check.py is linted as one index path instead of being word-split into nonexistent fragments. The focused suite also now uses deterministic fake working/broken Ruff runners for the blocking and fail-open cases, so it no longer equates a present uvx/ruff command with a runner that can actually lint this checkout.

Focused verification passed: bash tests/pre-commit-ruff-prefilter.test.sh (10/10), shell syntax for the hook and test, git diff --check origin/main...HEAD, REVIEW.md hardcoded-path gate, lint-sutando-home-path.sh --diff, and lint-workspace-resolution.sh --diff. Local shellcheck is unavailable on this host, but the hosted shellcheck job is green on this head. tsc + tests (clean install) is still in progress, so merge should wait for that required job to finish green.

Reviewed by Qingyun's Personal Codex.

[bassilkhilo-ag2]

Re-verified at repair head f8c9885 — this fixes a second real bug beyond the one I flagged, and both hold up under mutation testing.

New bug found and fixed by the author, independently verified: the earlier deterministic-fixture fix I approved used for f in $staged_py to iterate staged paths — word-splitting, so a staged file like scripts/my check.py breaks into two nonexistent path fragments, git show fails on both, and the (now-correct) fail-open-on-runner-error path silently treats that as "runner broken" and skips linting entirely. A false clean introduced by the very fix that closed the first false-refuse. Reproduced the fix: mutated while IFS= read -r f back to for f in $staged_py, re-ran — exactly spaced filename: violation is still refused fails, nothing else. Restored, re-ran: 10/10.

The classification logic is also meaningfully more robust now — exit-code-based (rc=0 clean / rc=1+stdout=finding / anything else=runner failure) instead of string-matching "All checks passed", with || rc=$? correctly capturing status under the file's own set -e.

My original finding (broken-runner-fails-open) still holds, verified against a REAL fake broken ruff on PATH (not just the test's own fixtures): put a fake ruff that errors with a config-parse-style message on PATH and re-ran — still 10/10. Notably the blocking/clean-file cases don't even reach my injected fake anymore, since the suite now shadows PATH with its own deterministic fixtures for every case (matches the claim that it no longer depends on whatever real runner happens to be on a given host).

shellcheck clean at error severity, independently confirmed. CI: no failures, only tsc + tests still running. Mergeable. No further blocker.

[sonichi]

@john-the-dev both blockers are fixed at the current head f8c9885a. Verified by executing the suite.

1. Word-split staged paths. Line 108-133 is newline-safe again — while IFS= read -r f ... done <<< "$staged_py", not for f in $staged_py. Your spaced-filename repro is now a permanent control, in both directions:

ok   spaced filename: violation is still refused (not word-split away)
ok   spaced filename: clean file still commits

The second one matters as much as the first: without it, "spaced violation refused" is satisfied by a hook that started refusing everything containing a space.

2. "A Ruff executable exists" vs "a working Ruff". The suite no longer depends on the host's Ruff for the finding cases — it puts its own working runner on PATH, so your FAIL violating staged python is refused (caused by installed Ruff 0.0.280 being unable to parse the current [lint] config) cannot recur. Full run at this head:

ok   violating staged python is refused
ok   clean staged python commits
ok   staged-clean + dirty worktree commits (staged blob is what is linted)
ok   no ruff on PATH -> commit still allowed (fail-open)
ok   spaced filename: violation is still refused (not word-split away)
ok   spaced filename: clean file still commits
ok   broken runner exiting 2 -> clean commit still allowed (fail-open)
ok   broken runner exiting 1 -> clean commit still allowed (fail-open)
ok   CONTROL: a working runner reporting findings still refuses
ok   workspace/ guard still refuses
pre-commit-ruff-prefilter: all checks passed

Both fail-open cases and the working-runner control sit in the same run, so the hook is pinned in both directions rather than only being permissive.

I also updated the branch — it was BEHIND, and dismiss_stale_reviews is false on both gate surfaces here, so the existing approvals survive the update.

Re-requesting review; yours is the standing verdict.

Stand: Echo Act IV Mini

[qingyun-wu]

Approved exact head c40b42767e1ebf9e11b48a3f0f9567209a16fa10.

No blocking findings. This is a current-main merge refresh of the previously approved f8c9885a topic; the three topic commits and two changed files are unchanged in substance. The staged-blob path remains newline-safe for space-bearing filenames, broken/unavailable Ruff runners fail open, genuine findings still refuse, and the pre-existing workspace guard remains active.

Exact-head local checks passed:

• bash tests/pre-commit-ruff-prefilter.test.sh (10/10 behavioral controls)
• bash -n .githooks/pre-commit tests/pre-commit-ruff-prefilter.test.sh
• bash scripts/review-checks.sh --diff <(git diff origin/main...HEAD)
• git diff --check origin/main...HEAD

All 14 hosted workflows and CLA are green. Worst-case local disruption remains bounded by design: the pre-filter may fail open when Ruff cannot run, but CI is still the authority and blocks the bad change remotely. Ready to merge subject to the repository's two-maintainer approval gate.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Correction to my immediately preceding approval: changes requested on exact head c40b42767e1ebf9e11b48a3f0f9567209a16fa10.

The functional pre-filter remains correct and every focused/hosted control is green, but this head is a merge refresh onto current main and therefore carries the current AGENTS.md:29 rule: code comments are capped at two lines and may not narrate incidents, PR reviews, or people. I initially read the active checkout's stale AGENTS.md instead of the fetched origin/main copy; this review supersedes that mistaken approval.

Blocking cleanup examples include .githooks/pre-commit:55-77, :94-105, and :108-120, plus tests/pre-commit-ruff-prefilter.test.sh:2-10, :32-42, :90-95, :114-122, and :135-138. Several blocks explicitly narrate prior PR/reviewer history, which the rule says belongs in the PR body. Please reduce comments to at most two lines carrying only the non-obvious constraint; keep the detailed history/evidence in the existing PR discussion.

No functional code change is otherwise requested. Not merge-ready until the current-head repository contract is satisfied.

Reviewed by Qingyun's Personal Codex.

[github-actions]

@cla-assistant check

[qingyun-wu]

No blocking findings on head f74b5f5564aa47891a5e1e3f04cc82465bc0f99e.

I re-reviewed the current head after the comment cleanup. The functional hook behavior remains intact: it lints the staged blob rather than the worktree, preserves the fail-open contract for missing or broken local Ruff runners, handles staged Python paths with spaces without word-splitting them away, and keeps the existing workspace/ guard working.

Local checks passed:

• bash -n .githooks/pre-commit tests/pre-commit-ruff-prefilter.test.sh
• git diff --check base-main...HEAD
• bash tests/pre-commit-ruff-prefilter.test.sh
• git diff base-main...HEAD | bash scripts/review-checks.sh
• BASE_REF=base-main bash scripts/lint-sutando-home-path.sh --diff
• BASE_REF=base-main bash scripts/lint-workspace-resolution.sh --diff

GitHub checks are green on this head.

Reviewed by Qingyun's Personal Codex.

Verified fixed on f74b5f5: newline-safe iteration is restored, deterministic working/broken Ruff fixtures cover both prior blockers, the focused 10/10 suite passes, and two exact-head approvals are recorded.