Skip to content

feat(hooks): agent quality rails — per-file lint/typecheck gate + uv guard - #2618

Open
kevinmessiaen wants to merge 8 commits into
mainfrom
feat/agent-quality-rails
Open

feat(hooks): agent quality rails — per-file lint/typecheck gate + uv guard#2618
kevinmessiaen wants to merge 8 commits into
mainfrom
feat/agent-quality-rails

Conversation

@kevinmessiaen

Copy link
Copy Markdown
Member

What

Two Claude Code hooks that turn documented-but-unenforced rules into mechanical guarantees, giving ~1s per-file feedback before make check.

  • .claude/hooks/quality_gate.py (PostToolUse, Edit|Write|MultiEdit) — runs ruff on edited libs/**/*.py; additionally basedpyright on libs/*/src/** only, exiting 2 with diagnostics so Claude self-corrects.
  • .claude/hooks/enforce_uv.py (PreToolUse, Bash) — blocks bare python/pytest, nudges to uv run.
  • .claude/settings.json — wires both. Replaces an inline one-liner that swallowed every failure with || true. SessionStart unchanged.

Why

make check runs basedpyright --level error . repo-wide as 1 of 7 targets, so type errors surfaced late and mixed with pre-existing ones. Ruff passing never meant CI was green.

Design decisions

Tests are never typechecked. TDD red-phase tests deliberately import code that doesn't exist yet — verified: a red-phase test yields errorCount: 1 (reportAttributeAccessIssue). Blocking it would break the workflow the gate protects. CI still typechecks tests, so the gap is intentional and one-directional: the hook never blocks what CI would pass.

--project is mandatory. basedpyright resolves config from cwd, not the target file. Measured on a clean source file:

Invocation errorCount
repo root, absolute path 0
foreign cwd, absolute path 3 (bogus reportMissingImports)
foreign cwd + --project 0

Without it the hook would block every source edit with fabricated errors.

Fail open, always — missing tooling, unset $CLAUDE_PROJECT_DIR, malformed input → exit 0. Fail closed only on genuine type errors. Gates on errorCount only, never warningCount (CI sets failOnWarnings: false).

The uv guard is deliberately narrow — anchored regex only. It won't catch cd x && pytest; a cleverer regex would false-block echo "run pytest". A guard that's occasionally silent beats one that blocks valid work.

Review findings (fixed)

Final review caught a Critical: both hooks crashed (exit 1) on malformed tool_input{"file_path":123}TypeError. Exit 1 means "non-blocking, user-visible", so a crashing hook neither blocked nor cleanly allowed; it spammed tracebacks while performing no checks. Fixed in 696c7cd with isinstance guards + a top-level catch-all. It survived initial testing because the matrix only covered well-formed payloads.

Verification

7/7 matrix passes, incl. both regressions (TDD red-phase → 0, foreign cwd → 0) and a negative control proving it genuinely blocks (real type error → exit 2 with file:line + reportAssignmentType, not a silent fail-open). 13 malformed shapes → all exit 0. make lint clean; make typecheck 0 errors 0 warnings.

Not verified: live in-session firing after a Claude Code restart — hooks were exercised by direct stdin invocation. Worth a sanity check on first use.

Unrelated: make check fails at security on a pre-existing setuptools 82.0.1 / PYSEC-2026-3447. This branch touches zero dependencies.

🤖 Generated with Claude Code

kevinmessiaen and others added 4 commits July 17, 2026 14:57
Blocks bare `python`/`pytest`, which fail with ModuleNotFoundError in this
repo, and nudges to `uv run`. Deliberately narrow: only anchored bare
invocations match, so legitimate commands like `echo "run pytest"` are not
false-blocked. Fails open on unparseable input.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Runs ruff on any edited libs/**/*.py, then basedpyright on source files
only, exiting 2 with diagnostics so Claude self-corrects before CI.

Tests under libs/*/tests are never typechecked: TDD red-phase tests
deliberately import code that does not exist yet, and blocking them would
break the workflow this gate exists to protect.

Passes --project explicitly because basedpyright resolves config from cwd,
not the target file; without it a clean source file reports bogus
reportMissingImports and every edit would be blocked.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replaces the inline PostToolUse one-liner that swallowed every failure with
`|| true`, so lint and type errors were never surfaced. Adds the PreToolUse
uv guard and extends the matcher to MultiEdit. SessionStart is unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Both hooks crashed with a traceback (exit 1) on unexpected payload shapes:
a non-string file_path raised TypeError at Path(), and a non-dict tool_input
raised AttributeError. Exit 1 means "non-blocking, user-visible", so a
crashing hook neither blocked nor cleanly allowed -- it spammed tracebacks on
every edit while performing no checks, violating the fail-open contract.

Adds explicit isinstance guards on tool_input/file_path/command plus a
top-level catch-all, so future tool-input shape drift cannot wedge the agent.

Also documents that CI typechecks tests while the hook does not: the gap is
intentional and one-directional, so the hook never blocks what CI would pass.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces Claude hooks to enforce development quality gates. It adds a PreToolUse hook (enforce_uv.py) to block bare python or pytest commands in favor of uv run, and a PostToolUse hook (quality_gate.py) to automatically run ruff and basedpyright on edited library files. The review feedback highlights critical robustness improvements for the quality gate hook: resolving relative file paths against the project root, using uv run instead of uv tool run for consistent tool versions, and implementing defensive dictionary parsing to prevent potential AttributeError crashes when processing diagnostics.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +55 to +58
path = Path(raw_path)

if path.suffix != ".py" or not path.exists():
return 0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

If raw_path is a relative path, checking path.exists() and resolving path.resolve() will be done relative to the current working directory (CWD) of the hook process. If the hook is executed from a different directory, path.exists() will incorrectly return False (causing a silent fail-open), or path.resolve().relative_to(...) will raise a ValueError (also failing open).

To make the hook robust against any CWD, we should resolve path relative to root if it is not already absolute.

        path = Path(raw_path)
        if not path.is_absolute():
            path = root / path

        if path.suffix != ".py" or not path.exists():
            return 0

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 70e6862 — relative paths are now anchored to $CLAUDE_PROJECT_DIR via root / raw_path before exists() / relative_to.

Comment thread .claude/hooks/quality_gate.py Outdated
Comment on lines +71 to +72
run(["uv", "tool", "run", "ruff", "format", str(path)], root)
run(["uv", "tool", "run", "ruff", "check", "--fix", str(path)], root)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using uv tool run ruff runs ruff from an isolated tool environment, which can introduce overhead and potentially use a different version of ruff than the one pinned in the project's virtual environment (as synced via uv sync).

Since the project's Makefile targets (lint and format) use uv run ruff, we know ruff is available in the project's virtual environment. Using uv run ruff is faster and guarantees version consistency with the rest of the repository.

Suggested change
run(["uv", "tool", "run", "ruff", "format", str(path)], root)
run(["uv", "tool", "run", "ruff", "check", "--fix", str(path)], root)
run(["uv", "run", "ruff", "format", str(path)], root)
run(["uv", "run", "ruff", "check", "--fix", str(path)], root)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 70e6862 — switched ruff and basedpyright to uv run to match the Makefile and the pinned project env.

Comment on lines +112 to +118
for diag in diagnostics:
if diag.get("severity") != "error":
continue
line_no = diag.get("range", {}).get("start", {}).get("line", 0) + 1
rule = diag.get("rule", "")
suffix = f" [{rule}]" if rule else ""
lines.append(f" {rel}:{line_no} {diag.get('message', '')}{suffix}")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

In Python, dict.get(key, default) returns None if the key exists but its value is explicitly set to None (e.g., "range": null in JSON). If diag.get("range") is None, calling .get("start") on it will raise an AttributeError, causing the hook to crash and silently fail open via the top-level except Exception block.

To ensure robust defensive programming, we should use or {} to guard against None values, and verify that diag is indeed a dictionary.

        for diag in diagnostics:
            if not isinstance(diag, dict) or diag.get("severity") != "error":
                continue
            range_val = diag.get("range") or {}
            start_val = range_val.get("start") or {}
            line_no = start_val.get("line", 0) + 1
            rule = diag.get("rule", "")
            suffix = f" [{rule}]" if rule else ""
            lines.append(f"  {rel}:{line_no} {diag.get('message', '')}{suffix}")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 70e6862 — diagnostics loop now guards with isinstance(diag, dict) and or {} on range / start so null JSON fields cannot AttributeError into the fail-open catch-all.

@davidberenstein1957

Copy link
Copy Markdown
Member

Addressed the review notes in 70e6862:

  1. Relative file_path values resolve against $CLAUDE_PROJECT_DIR, not hook CWD
  2. uv tool runuv run for ruff and basedpyright (pinned project env, same as Makefile)
  3. Defensive diagnostic parsing when range / start are null

@davidberenstein1957
davidberenstein1957 self-requested a review July 29, 2026 12:57
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>

@davidberenstein1957 davidberenstein1957 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation automation setup. Formatting fixed. Ready.

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>

@davidberenstein1957 davidberenstein1957 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs automation setup. Formatting & merge conflicts resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

2 participants