learningcircuit--local-deep-research/docs/decisions/0002-pre-commit-hook-reviews.md

# 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](https://github.com/codespell-project/codespell/issues/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.yaml` comments 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

1. **Fast**: target under 3 seconds for a typical staged-file set.
2. **Scoped appropriately**: file-scoped hooks are preferred. Hooks requiring
   full-repo analysis need a narrow `files:` trigger and team consensus.
3. **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).
4. **Validated**: run the hook against the existing codebase before proposing.
5. **Low false-positive rate**: if suppressions are needed from day one,
   reconsider whether the hook belongs here.
6. **Low false-positive on identifiers**: tools designed for natural language
   may over-fire on camelCase identifiers, abbreviations, and domain terms.
7. **Self-contained**: hooks must work in pre-commit's CI workflow without
   requiring system-level dependencies (`language: python` with
   `additional_dependencies`, not `language: system`).

## Related PRs

| PR | Title | Decision |
|----|-------|----------|
| [#3218](https://github.com/LearningCircuit/local-deep-research/pull/3218) | chore(hooks): prevent frozen utcnow() | Rejected — conflicts with existing hook |
| [#3219](https://github.com/LearningCircuit/local-deep-research/pull/3219) | chore(hooks): layer-import boundary enforcement | Accepted (fix: `exclude: ^tests/`) |
| [#3220](https://github.com/LearningCircuit/local-deep-research/pull/3220) | fix(security): escape server data in innerHTML | Accepted |
| [#3221](https://github.com/LearningCircuit/local-deep-research/pull/3221) | fix(config): sync ruff version | Accepted (fix: full `ruff format .`) |
| [#3222](https://github.com/LearningCircuit/local-deep-research/pull/3222) | chore(hooks): add codespell typo detection | Rejected — CamelCase false positives |
| [#3225](https://github.com/LearningCircuit/local-deep-research/pull/3225) | chore(hooks): `raise ... from` in except blocks | Accepted |
| [#3227](https://github.com/LearningCircuit/local-deep-research/pull/3227) | chore(hooks): detect unescaped innerHTML | Rejected — regex bug |
| [#3230](https://github.com/LearningCircuit/local-deep-research/pull/3230) | chore(hooks): vulture dead code detection | Rejected — policy escalation |
| [#3231](https://github.com/LearningCircuit/local-deep-research/pull/3231) | chore(hooks): mypy type checking | Rejected — already in CI |