_clean_one() folded three different outcomes into one "skipped" bucket, and
main() returned 0 whenever changed_paths was empty. A clean_file.py that
raised, was killed, or died before writing its JSON therefore produced exit
0, the hook passed, and the uncleaned file went into the commit. pre-commit
only surfaces a passing hook's output in verbose mode, so the traceback on
stderr was invisible in a normal run: the hook silently failed open, against
the contract its own docstring states ("Exit 0 means every file was already
clean").
Judge the run by its report, not by its exit code
-------------------------------------------------
The discriminator cannot be "returncode != 0". clean_file.py deliberately
exits 1 for a *successful* clean that left residual signals behind, prints
valid JSON while doing so, and tests/test_json_exit_code.py pins that. So a
non-zero code is not by itself a failure. What actually separates the cases
is whether the child produced a parsable report: no report means it never
reached its write, and that file is still marked. rc == 2 keeps its meaning
(unrecognized format / oversized input) and still skips, unchanged.
Why exit 3 (EXIT_PARTIAL) and not 1 or 2
-----------------------------------------
1 is taken, and taking it again would be a lie: it means "I rewrote your
files, review the diff and re-stage", so reusing it for a crash sends the
developer looking for a diff that does not exist. 2 is usage/refusal — a bad
invocation — but the invocation was valid; it is also the code clean_file.py
returns for the skip we must keep reporting as 0, so overloading it invites
exactly the confusion this commit removes. 3 is EXIT_PARTIAL, already
defined in common.py as "partial scan, some files failed to scan; takes
precedence over actionable findings", and already used with that meaning by
audit_dir.py:162 and audit_website.py:555. A batch that could not process
some of its files is precisely a partial run, and the documented precedence
resolves the mixed case for free: a batch that cleaned one file and failed
on another exits 3, while still reporting both. pre-commit fails the hook on
any non-zero, so the code is for humans and direct/CI callers — which is the
reason to follow the convention the repo already has rather than invent one.
Reporting
---------
Failures get the summary block the sibling hooks use (headline, indented
path, indented reason) instead of a raw traceback behind a path prefix. Only
the last stderr line is quoted, which for an uncaught exception is the line
that names the cause; a child killed by a signal leaves no stderr, so the
exit status is reported instead.
The leftover .bak is named, not deleted
----------------------------------------
clean_file.py --in-place backs the original up (clean_file.py:100) before
the write that failed, so every failed run leaves a sidecar. This wrapper
does not remove it: it cannot tell its own leftover from a .bak the
developer already had staged, and deleting the wrong file from inside an
already-failing hook is the worse outcome. Nothing is lost by keeping it —
safe_write_bytes() replaces atomically or not at all, so the original is
intact and the sidecar is redundant rather than load-bearing. The report
names the path when it exists so the developer is not left wondering where
it came from. Not taking the backup until it is needed is a change to
clean_file.py's own --in-place ordering and belongs with that file.
Tests cover the crash, empty-stdout, malformed-JSON and killed-child paths,
the mixed cleaned-plus-failed precedence, an end-to-end symlink case that
needs no mocking (common.py:256 refuses to write through a symlink), and two
regressions: the intended rc == 2 skip still exits 0, and a residual-signal
clean is still reported as a clean, not a failure.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Guillaume Meyer (The Opinionated Man) <1385518+guillaumemeyer@users.noreply.github.com>
CI gating for AI provenance marks already exists (audit_dir.py's -j
concurrency + SARIF export from #101), but that only runs after a
marked file has already been committed and pushed. Catch it at commit
time instead, using git's own hook point.
Adds two hooks via .pre-commit-hooks.yaml:
- watermarks-remover-check: fails the commit and lists findings when
staged files carry AI/C2PA marks. Wraps audit_lib.scan_file() /
is_actionable() -- the exact per-file logic audit_dir.py already
uses for CI, so the pre-commit gate and the CI gate agree on what
counts as actionable.
- watermarks-remover-clean (opt-in): rewrites staged files in place by
shelling out to clean_file.py --in-place per file (no duplicated
cleaning logic), then exits 1 so the developer reviews the diff and
re-stages -- the same convention as auto-fixing hooks like ruff --fix.
.pre-commit-hooks.yaml needed an explicit allow-rule in the deny-by-
default .gitignore, same as every other root-level config file already
listed there.
Closes#135
Co-authored-by: Guillaume Meyer (The Opinionated Man) <1385518+guillaumemeyer@users.noreply.github.com>
