wehub-resource-sync
7a0da7932b
5.9 KiB
ADR-0002: Pre-commit hook batch review
Date: 2026-03-28 Status: Accepted
Context
Nine PRs proposing new or modified pre-commit hooks were evaluated simultaneously. The repository already has 43 pre-commit hooks; new hooks must clear a high bar for signal-to-noise ratio, speed, and non-duplication with CI.
Review was conducted using 86 automated review agents across 4 rounds, cross-validating findings and verifying correctness of each recommendation.
Decision
Accepted (4 PRs)
| PR | Hook | Fix Applied |
|---|---|---|
| #3220 | fix(security): escape server data in innerHTML | None — merged as-is |
| #3221 | fix(config): sync ruff version (v0.14.10 → v0.15.8) | Full ruff format . pass to reformat all old-style lambdas |
| #3225 | chore(hooks): raise ... from enforcement in except blocks |
None — merged as-is |
| #3219 | chore(hooks): layer-import boundary enforcement | Added exclude: ^tests/ to prevent false positives on test files |
Rejected (5 PRs)
#3218 — utcnow callable check: Directly contradicts the existing
check-utcnow-parens hook. sqlalchemy_utc.utcnow is a FunctionElement
class, so utcnow() creates a SQL expression rendered per-INSERT as
CURRENT_TIMESTAMP. The existing hook correctly requires parentheses. Having
both hooks would block every commit.
#3222 — codespell (typo detection): False-positives on camelCase identifiers
are a known, unsolved problem
(codespell-project/codespell#196,
open since 2017). The tool treats hasTable, doesnt, crasher, and similar
code identifiers as misspellings, requiring an ever-growing ignore-words-list.
#3227 — innerHTML regex scanner: The core regex (INTERPOLATION_RE using
[^}]+) truncates at the first }, failing on nested braces in template
literals. Multiple real JS files are affected: settings.js:80,
collection_details.js:210, history.js:421,501,503,
semantic_search.js:197-205. This causes both false positives and false
negatives — unacceptable for a security tool. The project already has ESLint and
Bearer for JavaScript security analysis, both of which use proper AST-based
parsing. Use ESLint no-unsanitized rule instead.
#3230 — vulture (dead code detection): The hook used language: system,
which requires vulture on the system PATH and breaks in CI (the pre-commit
workflow does not install project dev dependencies). The larger issue is policy:
vulture is advisory-only in CI (release gate, not PR gate). Promoting it to a
blocking pre-commit hook is a policy escalation that should be a deliberate team
decision. Use the existing vulture-dead-code.yml CI workflow instead.
#3231 — mypy (type checking): Most production modules have
ignore_errors = true in pyproject.toml (web.*, database.*, settings.*,
utilities.*, security.*, api.*, and 8+ more). mypy already blocks PRs via
mypy-type-check.yml + ci-gate.yml. Adding it as a pre-commit hook would add
10-30 seconds per commit for near-zero detection value on the majority of the
codebase.
Consequences
- Four new hooks strengthen the pre-commit pipeline (XSS prevention, ruff consistency, exception chaining, architectural boundaries)
- Five proposals are documented as rejected with specific technical reasons, preventing re-proposals without new information
- Rejected hooks are listed in
.pre-commit-config.yamlcomments with a link to this document - The total hook count increases from 43 to 45, with negligible performance impact (~100-200ms per commit for the new hooks)
Principles for future hook proposals
- Fast: target under 3 seconds for a typical staged-file set.
- Scoped appropriately: file-scoped hooks are preferred. Hooks requiring
full-repo analysis need a narrow
files:trigger and team consensus. - High local value: don't add slow CI checks to pre-commit when the local feedback value is low (e.g., mypy with most modules ignored).
- Validated: run the hook against the existing codebase before proposing.
- Low false-positive rate: if suppressions are needed from day one, reconsider whether the hook belongs here.
- Low false-positive on identifiers: tools designed for natural language may over-fire on camelCase identifiers, abbreviations, and domain terms.
- Self-contained: hooks must work in pre-commit's CI workflow without
requiring system-level dependencies (
language: pythonwithadditional_dependencies, notlanguage: system).
Related PRs
| PR | Title | Decision |
|---|---|---|
| #3218 | chore(hooks): prevent frozen utcnow() | Rejected — conflicts with existing hook |
| #3219 | chore(hooks): layer-import boundary enforcement | Accepted (fix: exclude: ^tests/) |
| #3220 | fix(security): escape server data in innerHTML | Accepted |
| #3221 | fix(config): sync ruff version | Accepted (fix: full ruff format .) |
| #3222 | chore(hooks): add codespell typo detection | Rejected — CamelCase false positives |
| #3225 | chore(hooks): raise ... from in except blocks |
Accepted |
| #3227 | chore(hooks): detect unescaped innerHTML | Rejected — regex bug |
| #3230 | chore(hooks): vulture dead code detection | Rejected — policy escalation |
| #3231 | chore(hooks): mypy type checking | Rejected — already in CI |